feat(preferences): add app settings for new proxy options COMPASS-8141 #6099

addaleax · 2024-08-08T14:41:05Z

This isn't hooked up to any actual proxy logic yet, to be clear 🙂

addaleax · 2024-08-08T14:41:24Z

packages/compass-preferences-model/src/preferences-persistent-storage.spec.ts

@@ -22,7 +28,7 @@ describe('PersistentStorage', function () {
  });

  afterEach(async function () {
-    await fs.rmdir(tmpDir, { recursive: true });
+    await fs.rm(tmpDir, { recursive: true, force: true });


Drive-by: This fixes a deprecation warning

addaleax · 2024-08-08T14:42:40Z

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

+        );
+      },
+    },
+  },


This is one of the main two things that I'd like feedback on – how do we feel about this approach to separating secrets from non-secrets in preferences? This is unfortunately not something I considered a lot in the tech design, and I did try another approach first (splitting these into separate preference entries and doing the encryption/decryption in the code that accesses these), but so far it's something I think works pretty well

addaleax · 2024-08-08T14:44:17Z

packages/compass-settings/src/stores/settings.ts

  return async (dispatch) => {
-    dispatch({ type: ActionTypes.OpenSettingsModal });
+    dispatch({ type: ActionTypes.OpenSettingsModal, tab });


... and this the other thing, I guess, although I don't know much about what potential alternatives would look like – this is moving the "open tab" state to redux from the React component, and this will enable us to open a specific tab in the modal from other parts of Compass (which is planned for the next PR)

Yeah, that's definitely the way to go about it. We can also now change the welcome modal to link directly to privacy tab if we want to

Oh, yeah, that makes a ton of sense 🙂 Done in 20a2084!

addaleax · 2024-08-12T12:08:57Z

packages/compass-e2e-tests/helpers/compass.ts

@@ -472,7 +472,8 @@ async function getCompassExecutionParameters(): Promise<{
  );
  const binary = testPackagedApp
    ? getCompassBinPath(await getCompassBuildMetadata())
-    : require('electron');
+    : // eslint-disable-next-line @typescript-eslint/no-var-requires
+      (require('electron') as unknown as string);


Really unsure why this started popping up as an issue for me tbh – feels like it should always have been complaining about this

addaleax · 2024-08-12T12:09:09Z

packages/compass-e2e-tests/tests/auto-connect.test.ts

@@ -245,7 +245,7 @@ describe('Automatically connecting from the command line', function () {
      await browser.waitForConnectionResult('success');
      await browser.execute(() => {
        // eslint-disable-next-line @typescript-eslint/no-var-requires
-        require('electron').ipcRenderer.call('test:show-connect-window');
+        require('electron').ipcRenderer.invoke('test:show-connect-window');


Similarly here, this is unrelated but ~~was required for me to be able to run e2e tests at all~~ changed this to use as any to ignore a TS error that started showing up for some reason

gribnoysup · 2024-08-12T14:58:38Z

packages/compass-settings/src/stores/settings.ts

      };
    case ActionTypes.CloseSettingsModal:
      return {
        ...state,
        isModalOpen: false,
+        tab: undefined,


Small nit: because of how Settings modal (or rather any modal in leafygreen) works it's usually more preferable to reset modal related state before open and not on close. The reason for this is that the modal is still on the screen, animating away when this state changes, causing slightly weird jumps in UI during the animation. So as a suggestion, instead of resetting it here, do this in the handler for the OpenSettingsModal action unless there is a value provided in the open action

Thanks for explaining! Absolutely makes sense, done in 2d77051 :)

gribnoysup · 2024-08-12T15:00:47Z

packages/compass-settings/src/stores/settings.ts


-export const reducer: Reducer<State> = (state = INITIAL_STATE, action) => {
+export const reducer: Reducer<State, Actions> = (


I know it's a pain in the butt, but Redux docs recommend against unions for action type and we generally follow this rule by using isAction helper

Ah, right. I literally forgot that that's what we do in Compass tbh 🙈 Just saw that action wasn't typed and wanted to fix that, but I'll gladly remove this again and use isAction instead, will do that in the next commit.

It does make me wonder, should we maybe prefer typing our reducers with Reducer<State, Action> (instead of the implied default Reducer<State, AnyAction>)? That would still make sure that we're not accidentally relying on the any-ness of the action parameter we're using

Done in 8236817 👍

gribnoysup · 2024-08-12T15:01:27Z

packages/compass-settings/src/stores/settings.ts

      };
    case ActionTypes.ChangeFieldValue:
+      if (state.loadingState !== 'ready') return { ...state };


Nit: no need for shallow cloning if state is not changed

Suggested change

if (state.loadingState !== 'ready') return { ...state };

if (state.loadingState !== 'ready') return state;

Done in 8236817 👍

gribnoysup · 2024-08-12T15:03:52Z

packages/compass-settings/src/stores/settings.ts

  return async (dispatch) => {
-    dispatch({ type: ActionTypes.OpenSettingsModal });
+    dispatch({ type: ActionTypes.OpenSettingsModal, tab });


Yeah, that's definitely the way to go about it. We can also now change the welcome modal to link directly to privacy tab if we want to

gribnoysup · 2024-08-12T15:05:32Z

packages/compass-settings/src/stores/settings.ts

+): SettingsThunkAction<void, SelectTabAction> => {
+  return (dispatch) => {
+    dispatch({ type: ActionTypes.SelectTab, tab });
+  };
+};


Small nit: doesn't need to be a thunk action. Nothing wrong with it, just you usually might want to use those only if you need to dispatch multiple events from one action, access current state before dispatching, or have access to the thunk "extra arg". None of these are the case here

Suggested change

): SettingsThunkAction<void, SelectTabAction> => {

return (dispatch) => {

dispatch({ type: ActionTypes.SelectTab, tab });

};

};

): SelectTabAction => {

return { type: ActionTypes.SelectTab, tab }

};

Sure – done in b3a9e6f 👍

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

addaleax · 2024-08-13T15:04:52Z

configs/tsconfig-compass/tsconfig.common.json

+  "extends": "@mongodb-js/tsconfig-devtools/tsconfig.common.json",
+  "compilerOptions": {
+    "moduleResolution": "node16"
+  }


... so that the '@mongodb-js/devtools-proxy-support/proxy-options' import works (which in turn also causes the small change to the mongodb-explain-compat package.json).

Woah, last time I tried to turn it on, a bunch of other things broke, which are maybe gone now? This means that we can also remove weird workarounds for /provider re-exports everywhere, awesome!

Ah, shoot, just saw the last commit 😬

Fwiw, npm run bootstrap passed just fine – but this failed during the e2e test phase. Probably not hard to fix if we want to, though

feat(preferences): add app settings for new proxy options COMPASS-8141

0a266e5

github-actions bot added the feat label Aug 8, 2024

addaleax commented Aug 8, 2024

View reviewed changes

addaleax added the feature flagged PRs labeled with this label will not be included in the release notes of the next release label Aug 8, 2024

addaleax and others added 6 commits August 9, 2024 16:01

Merge branch 'main' into 8141-dev

76cb602

fixup: adjust existing e2e tests

9d9b8e1

fixup: dependency-inject safeStorage into compass-preferences-model

983dc54

Merge remote-tracking branch 'origin/main' into 8141-dev

e761ef5

fixup: resolve package-lock conflicts

5790143

fixup: fix TS error in tests?

f6705d0

addaleax commented Aug 12, 2024

View reviewed changes

addaleax added 2 commits August 12, 2024 15:44

fixup: .call is the right one, even with TS errors

e721d42

fixup: make depcheck happy and remove electron dependency entirely

fc8b3ae

gribnoysup reviewed Aug 12, 2024

View reviewed changes

addaleax added 6 commits August 12, 2024 20:44

fixup: isAction instead of union type

8236817

fixup: no thunk action

b3a9e6f

fixup: do not reset tab in close action

2d77051

fixup: open privacy tab when opening settings from welcome modal

20a2084

fixup: un-.only(), fix tests

3a435fc

Merge remote-tracking branch 'origin/main' into 8141-dev

eb1aa65

addaleax commented Aug 13, 2024

View reviewed changes

packages/compass-preferences-model/src/preferences-schema.ts Outdated Show resolved Hide resolved

fixup: bump devtools-proxy-support, specify node16 module resolution

5e4af0b

addaleax commented Aug 13, 2024

View reviewed changes

fixup: scope moduleResolution: node16 to only compass-preferences-model

e058474

addaleax requested a review from gribnoysup August 13, 2024 16:33

gribnoysup approved these changes Aug 14, 2024

View reviewed changes

addaleax merged commit 01cf750 into main Aug 14, 2024
28 checks passed

addaleax deleted the 8141-dev branch August 14, 2024 10:10


		export const reducer: Reducer<State> = (state = INITIAL_STATE, action) => {
		export const reducer: Reducer<State, Actions> = (

	if (state.loadingState !== 'ready') return { ...state };
	if (state.loadingState !== 'ready') return state;

feat(preferences): add app settings for new proxy options COMPASS-8141 #6099

feat(preferences): add app settings for new proxy options COMPASS-8141 #6099

Uh oh!

Conversation

addaleax commented Aug 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax Aug 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gribnoysup Aug 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gribnoysup Aug 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

addaleax commented Aug 8, 2024 •

edited

Loading

addaleax Aug 12, 2024 •

edited

Loading

gribnoysup Aug 12, 2024 •

edited

Loading

gribnoysup Aug 13, 2024 •

edited

Loading