chore(compass-settings): update settings list and input component to …

…have more granular state subscription COMPASS-7098 (#4735)

* chore(compass-settings): update settings list and input component to have more granular state subscription

* chore(mocha-config-compass): set up preferences for all electron test environments

configs/mocha-config-compass/main-process.js

@@ -5,3 +5,13 @@ app.on('web-contents-created', function (_, webContents) {
   enable(webContents);
 });
 initialize();
+// Every compass plugin depends on compass-preferences-model, for a lot of them
+// running tests in electron environments are either spamming "no handler
+// registered" warnings or just not working when expected settings are missing
+// or can't be updated. To handle that we are setting up preferences for every
+// test environment, but making sure that it's in sandbox mode so that nothing
+// is actually written to the disk when preferences change
+process.env.COMPASS_TEST_USE_PREFERENCES_SANDBOX =
+  process.env.COMPASS_TEST_USE_PREFERENCES_SANDBOX ?? 'true';
+// NB: Not adding this as a dep in package.json to avoid circular dependency
+require('compass-preferences-model').setupPreferences();

configs/mocha-config-compass/register/electron-renderer-register.js

@@ -27,6 +27,11 @@ if (isElectronRenderer) {
           'console-call',
           k,
           args.map((arg) => {
+            if (
+              ['string', 'number', 'boolean', 'undefined'].includes(typeof arg)
+            ) {
+              return arg;
+            }
             return inspect(arg);
           })
         );

packages/compass-preferences-model/src/index.ts

@@ -15,7 +15,7 @@ export type {
   AllPreferences,
   Preferences,
 };
-import { preferencesMain } from './setup-preferences';
+import { preferencesMain, setupPreferences } from './setup-preferences';
 import { preferencesIpc } from './renderer-ipc';
 export {
   parseAndValidateGlobalPreferences,
@@ -46,7 +46,7 @@ export interface PreferencesAccess {
   ): () => void;
   createSandbox(): Promise<PreferencesAccess>;
 }
-
+export { setupPreferences };
 export const preferencesAccess: PreferencesAccess =
   preferencesIpc ?? preferencesMain;
 export default preferencesAccess;

packages/compass-preferences-model/src/setup-preferences.ts

@@ -23,7 +23,8 @@ export async function setupPreferences(
 
   const preferences = (preferencesSingleton = new Preferences(
     getStoragePaths()?.basepath,
-    globalPreferences
+    globalPreferences,
+    process.env.COMPASS_TEST_USE_PREFERENCES_SANDBOX === 'true'
   ));
 
   await preferences.setupStorage();

packages/compass-query-bar/src/components/query-ai.spec.tsx

@@ -1,6 +1,12 @@
 import React from 'react';
 import type { ComponentProps } from 'react';
-import { cleanup, fireEvent, render, screen } from '@testing-library/react';
+import {
+  cleanup,
+  fireEvent,
+  render,
+  screen,
+  waitFor,
+} from '@testing-library/react';
 import { expect } from 'chai';
 import sinon from 'sinon';
 import type { SinonSpy } from 'sinon';
@@ -14,6 +20,7 @@ import {
 } from '../stores/ai-query-reducer';
 import { DEFAULT_FIELD_VALUES } from '../constants/query-bar-store';
 import { mapQueryToFormFields } from '../utils/query';
+import preferencesAccess from 'compass-preferences-model';
 
 const noop = () => {
   /* no op */
@@ -74,8 +81,18 @@ describe('QueryAI Component', function () {
   });
 
   describe('Query AI Feedback', function () {
-    beforeEach(function () {
+    let trackUsageStatistics: boolean | undefined;
+
+    beforeEach(async function () {
       store = renderQueryAI();
+      trackUsageStatistics =
+        preferencesAccess.getPreferences().trackUsageStatistics;
+      // 'compass:track' will only emit if tracking is enabled
+      await preferencesAccess.savePreferences({ trackUsageStatistics: true });
+    });
+
+    afterEach(async function () {
+      await preferencesAccess.savePreferences({ trackUsageStatistics });
     });
 
     it('should log a telemetry event with the entered text on submit', async function () {
@@ -107,21 +124,22 @@ describe('QueryAI Component', function () {
 
       screen.getByText('Submit').click();
 
-      // Let the track event occur.
-      await new Promise((resolve) => setTimeout(resolve, 6));
-
-      // No feedback popover is shown.
-      expect(screen.queryByTestId(feedbackPopoverTextAreaId)).to.not.exist;
-
-      expect(trackingLogs).to.deep.equal([
-        {
-          event: 'AIQuery Feedback',
-          properties: {
-            feedback: 'positive',
-            text: 'this is the query I was looking for',
-          },
+      await waitFor(
+        () => {
+          // No feedback popover is shown.
+          expect(screen.queryByTestId(feedbackPopoverTextAreaId)).to.not.exist;
+          expect(trackingLogs).to.deep.equal([
+            {
+              event: 'AIQuery Feedback',
+              properties: {
+                feedback: 'positive',
+                text: 'this is the query I was looking for',
+              },
+            },
+          ]);
         },
-      ]);
+        { interval: 10 }
+      );
     });
   });
 });

packages/compass-settings/src/components/settings/feature-preview.tsx

@@ -1,10 +1,5 @@
 import React from 'react';
-import { connect } from 'react-redux';
-import type { RootState } from '../../stores';
-import { changeFieldValue } from '../../stores/settings';
-import type { SettingsListProps } from './settings-list';
-import { SettingsList } from './settings-list';
-import { pick } from '../../utils/pick';
+import SettingsList from './settings-list';
 import preferences, {
   usePreference,
   featureFlags,
@@ -22,12 +17,6 @@ const developmentFeatureFlagFields = featureFlagFields.filter(
   (k: keyof typeof featureFlags) => featureFlags[k].stage === 'development'
 );
 
-type FeatureFlagFields = typeof featureFlagFields[number];
-type FeaturePreviewSettingsProps = Omit<
-  SettingsListProps<FeatureFlagFields>,
-  'fields'
->;
-
 // Lets us call `setShowDevFeatureFlags(true | false)` from DevTools.
 (globalThis as any).setShowDevFeatureFlags = async (
   showDevFeatureFlags = true
@@ -63,9 +52,7 @@ export function useShouldShowFeaturePreviewSettings(): boolean {
   return showPreviewFeatures || showDevFeatures;
 }
 
-export const FeaturePreviewSettings: React.FunctionComponent<
-  FeaturePreviewSettingsProps
-> = ({ ...props }) => {
+export const FeaturePreviewSettings: React.FunctionComponent = () => {
   const showPreviewFeatures = useShouldShowPreviewFeatures();
   const showDevFeatures = useShouldShowDevFeatures();
 
@@ -77,23 +64,14 @@ export const FeaturePreviewSettings: React.FunctionComponent<
       </div>
 
       {showPreviewFeatures && (
-        <SettingsList fields={previewFeatureFlagFields} {...props} />
+        <SettingsList fields={previewFeatureFlagFields} />
       )}
 
       {showDevFeatures && (
-        <SettingsList fields={developmentFeatureFlagFields} {...props} />
+        <SettingsList fields={developmentFeatureFlagFields} />
       )}
     </div>
   );
 };
 
-const mapState = ({ settings: { settings, preferenceStates } }: RootState) => ({
-  currentValues: pick(settings, featureFlagFields),
-  preferenceStates: pick(preferenceStates, featureFlagFields),
-});
-
-const mapDispatch = {
-  onChange: changeFieldValue,
-};
-
-export default connect(mapState, mapDispatch)(FeaturePreviewSettings);
+export default FeaturePreviewSettings;

packages/compass-settings/src/components/settings/general.spec.tsx

@@ -1,34 +1,36 @@
 import React from 'react';
-import { render, screen, within } from '@testing-library/react';
+import { cleanup, render, screen, within } from '@testing-library/react';
 import userEvent from '@testing-library/user-event';
-import { stub } from 'sinon';
 import { expect } from 'chai';
-
+import { Provider } from 'react-redux';
 import { GeneralSettings } from './general';
+import { configureStore } from '../../stores';
+import { fetchSettings } from '../../stores/settings';
 
-describe('GeneralsSettings', function () {
+describe('GeneralSettings', function () {
   let container: HTMLElement;
-  let onChangeSpy: sinon.SinonStub;
-  let currentValues: any;
+  let store: ReturnType<typeof configureStore>;
+
+  function getSettings() {
+    return store.getState().settings.settings;
+  }
 
-  beforeEach(function () {
-    currentValues = {};
-    onChangeSpy = stub();
+  beforeEach(async function () {
+    store = configureStore();
+    await store.dispatch(fetchSettings());
     const component = () => (
-      <GeneralSettings
-        onChange={onChangeSpy}
-        preferenceStates={{}}
-        currentValues={currentValues}
-      />
+      <Provider store={store}>
+        <GeneralSettings />
+      </Provider>
     );
-    const { rerender } = render(component());
-    onChangeSpy.callsFake((option, value) => {
-      currentValues[option] = value;
-      rerender(component());
-    });
+    render(component());
     container = screen.getByTestId('general-settings');
   });
 
+  afterEach(function () {
+    cleanup();
+  });
+
   [
     'readOnly',
     'enableShell',
@@ -38,30 +40,26 @@ describe('GeneralsSettings', function () {
     it(`renders ${option}`, function () {
       expect(within(container).getByTestId(option)).to.exist;
     });
-    it(`calls onChange when ${option} is changed`, function () {
-      expect(onChangeSpy).to.not.have.been.called;
+    it(`changes ${option} value when option is clicked`, function () {
       const checkbox = within(container).getByTestId(option);
+      const initialValue = getSettings()[option];
       userEvent.click(checkbox, undefined, {
         skipPointerEventsCheck: true,
       });
-      expect(onChangeSpy).to.have.been.calledOnceWithExactly(option, true);
-      expect(currentValues[option]).to.equal(true);
+      expect(getSettings()).to.have.property(option, !initialValue);
     });
   });
 
   ['maxTimeMS'].forEach((option) => {
     it(`renders ${option}`, function () {
       expect(within(container).getByTestId(option)).to.exist;
     });
-    it(`calls onChange when ${option} is changed`, function () {
-      expect(onChangeSpy).to.not.have.been.called;
+    it(`changes ${option} value when typing in the input`, function () {
       const field = within(container).getByTestId(option);
       userEvent.type(field, '42');
-      expect(onChangeSpy).to.have.been.calledWithExactly(option, 42);
-      expect(currentValues[option]).to.equal(42);
+      expect(getSettings()).to.have.property(option, 42);
       userEvent.clear(field);
-      expect(onChangeSpy).to.have.been.calledWithExactly(option, undefined);
-      expect(currentValues[option]).to.equal(undefined);
+      expect(getSettings()).to.have.property(option, undefined);
     });
   });
 });

packages/compass-settings/src/components/settings/general.tsx

@@ -1,10 +1,5 @@
 import React from 'react';
-import { connect } from 'react-redux';
-import type { RootState } from '../../stores';
-import { changeFieldValue } from '../../stores/settings';
-import type { SettingsListProps } from './settings-list';
-import { SettingsList } from './settings-list';
-import { pick } from '../../utils/pick';
+import SettingsList from './settings-list';
 
 const generalFields = [
   'readOnly',
@@ -17,30 +12,17 @@ const generalFields = [
     ? (['installURLHandlers'] as const)
     : []),
 ] as const;
-type GeneralFields = typeof generalFields[number];
-type GeneralSettingsProps = Omit<SettingsListProps<GeneralFields>, 'fields'>;
 
-export const GeneralSettings: React.FunctionComponent<GeneralSettingsProps> = (
-  props
-) => {
+export const GeneralSettings: React.FunctionComponent = () => {
   return (
     <div data-testid="general-settings">
       <div>
         To enhance the user experience, Compass can enable or disable particular
         features. Please choose from the settings below:
       </div>
-      <SettingsList fields={generalFields} {...props} />
+      <SettingsList fields={generalFields} />
     </div>
   );
 };
 
-const mapState = ({ settings: { settings, preferenceStates } }: RootState) => ({
-  currentValues: pick(settings, generalFields),
-  preferenceStates: pick(preferenceStates, generalFields),
-});
-
-const mapDispatch = {
-  onChange: changeFieldValue,
-};
-
-export default connect(mapState, mapDispatch)(GeneralSettings);
+export default GeneralSettings;

packages/compass-settings/src/components/settings/oidc-settings.spec.tsx

@@ -1,38 +1,47 @@
 import React from 'react';
-import { render, screen, within } from '@testing-library/react';
+import { cleanup, render, screen, within } from '@testing-library/react';
 import userEvent from '@testing-library/user-event';
-import { spy } from 'sinon';
 import { expect } from 'chai';
-
+import { Provider } from 'react-redux';
 import { OIDCSettings } from './oidc-settings';
+import { configureStore } from '../../stores';
+import { fetchSettings } from '../../stores/settings';
 
 describe('OIDCSettings', function () {
   let container: HTMLElement;
-  let onChangeSpy: sinon.SinonSpy;
+  let store: ReturnType<typeof configureStore>;
+
+  function getSettings() {
+    return store.getState().settings.settings;
+  }
 
-  beforeEach(function () {
-    onChangeSpy = spy();
-    render(
-      <OIDCSettings
-        onChange={onChangeSpy}
-        preferenceStates={{}}
-        currentValues={{} as any}
-      />
+  beforeEach(async function () {
+    store = configureStore();
+    await store.dispatch(fetchSettings());
+    const component = () => (
+      <Provider store={store}>
+        <OIDCSettings />
+      </Provider>
     );
+    render(component());
     container = screen.getByTestId('oidc-settings');
   });
 
-  ['showOIDCDeviceAuthFlow'].forEach((option) => {
+  afterEach(function () {
+    cleanup();
+  });
+
+  ['showOIDCDeviceAuthFlow', 'persistOIDCTokens'].forEach((option) => {
     it(`renders ${option}`, function () {
       expect(within(container).getByTestId(option)).to.exist;
     });
-    it(`calls onChange when ${option} is changed`, function () {
-      expect(onChangeSpy.calledOnce).to.be.false;
+    it(`changed ${option} value when option is clicked`, function () {
       const checkbox = within(container).getByTestId(option);
+      const initialValue = getSettings()[option];
       userEvent.click(checkbox, undefined, {
         skipPointerEventsCheck: true,
       });
-      expect(onChangeSpy.calledWith(option, true)).to.be.true;
+      expect(getSettings()).to.have.property(option, !initialValue);
     });
   });
 });