chore(intercom): Add request to the update server to determine if Intercom integration is allowed COMPASS-9371 #6982

kraenhansen · 2025-06-03T12:55:21Z

Description

Combined with https://github.com/10gen/compass-mongodb-com/pull/133, merging this will introduce a preflight request to the update server to determine if Compass is allowed to setup the Intercom integration.

TODO: We might want to send the Compass version and OS / arch to that endpoint, in order to be able to selectively respond based on those parameters. But is that a thing that we will reasonably be using?

Checklist

New tests and/or benchmarks are included
Documentation is changed or added
If this change updates the UI, screenshots/videos are added and a design review is requested
I have signed the MongoDB Contributor License Agreement (https://www.mongodb.com/legal/contributor-agreement)

Motivation and Context

Bugfix
New feature
Dependency update
Misc

Open Questions

Dependents

Types of changes

Backport Needed
Patch (non-breaking change which fixes an issue)
Minor (non-breaking change which adds functionality)
Major (fix or feature that would cause existing functionality to change)

kraenhansen · 2025-06-03T13:05:22Z

packages/compass-intercom/src/setup-intercom.ts

    return;
  }

+  if (!(await isIntercomAllowed())) {


TODO: We shouldn't send a request networkTraffic is disabled.

We also want to prevent a request if the enableFeedbackPanel preference is false, or we're on web right? I'm guessing the web part is something we get from only accessing this from the compass electron side.

enableFeedbackPanel depens on network traffic already, so checking just one should be enough. We might want to just move this check higher in this method maybe

I found a way to shuffle stuff around. I am checking networkTraffic explicitly to increase cohesion and avoid relying on an implicit fact (the enableFeedbackPanel depending on networkTraffic - as I could observe that, but personally failed to establish the reason for that as I was writing this).

I'd have a strong preference for us not to do that, deriveNetworkTrafficOptionState is a mechanism we specifically added so that feature flags for some features (like feedback panel) don't need to explicitly check for network traffic. I'm happy to discuss how to make it more visible if just looking at the preferences descriptions that use deriveNetworkTrafficOptionState is not enough, but introducing a different pattern just makes it more obscure. Looking at the code it's now not clear whether or not networkTraffic needs to be checked even if deriveNetworkTrafficOptionState is applied to the preference. If I look here, it needs to be, if I look at some other preferences it doesn't, and there's no explanation for why this is happening

I'll rely on enableFeedbackPanel directly then 👍 I seached for deriveNetworkTrafficOptionState and I don't know why I wasn't able to find that before.

Another alternative would be for the DeriveValueFunction to be async and call the endpoint from there, as to completely disable the feature based on a response from the update server. But that might be more involved 🤔

kraenhansen · 2025-06-06T14:53:22Z

I'm seeing some pretty consistent failures of the e2e tests related to dynamically changing the proxy preference, but I have no idea why 😬 I can reproduce it locally and it doesn't reproduce off main locally either.

https://parsley.mongodb.com/test/10gen_compass_main_test_packaged_app_rhel_test_packaged_app_1_patch_1fd78e7d7f2c6c69213ea58b921c82f1f660fa22_6842eb5c66fdd10007816d59_25_06_06_13_21_35/0/0bbe998191287f5102298f00bf09d948?bookmarks=0,15&shareLine=0

kraenhansen · 2025-06-10T11:16:25Z

Turns out the preflight request opened a connection which made the proxy tests fail when they requests afterwards.

kraenhansen · 2025-06-11T17:52:28Z

packages/compass-intercom/src/setup-intercom.spec.ts

-    // NOTE: we use 301 since intercom will redirects
-    // to the actual location of the widget script
-    fetchMock.resolves({ status: 301 } as Response);
+    fetchMock = sinon.stub(globalThis, 'fetch');


Using this makes it possible to restore afterwards.

kraenhansen · 2025-06-11T17:53:29Z

packages/compass/src/app/utils/csp.ts

    // Used by proxy tests, since Chrome does not like proxying localhost
    // (this does not result in actual outgoing HTTP requests)
-    extraAllowed.push('http://compass.mongodb.com/');
+    extraAllowed.push('http://proxy-test-compass.mongodb.com/');


I'm changing this to avoid conflicts with the actual compass update server URL called when setting up intercom.

addaleax · 2025-06-12T12:53:27Z

packages/compass-intercom/src/setup-intercom.ts

+    await toggleEnableFeedbackPanel(shouldLoad);
+  } catch (error) {
+    debug('initial toggle failed', {
+      error: error instanceof Error && error.message,


Why not just

Suggested change

error: error instanceof Error && error.message,

error

?

addaleax · 2025-06-12T12:53:36Z

packages/compass-intercom/src/setup-intercom.ts

+        (error) => {
+          debug(
+            'Failed to fetch intercom integration status, defaulting to false',
+            { error: error instanceof Error && error.message }


ditto

Suggested change

{ error: error instanceof Error && error.message }

{ error }

addaleax · 2025-06-12T12:54:47Z

packages/compass-intercom/src/setup-intercom.ts

  );
 }
+
+function isIntercomAllowed(allowNetworkTraffic = true): Promise<boolean> {


(I think we'd want to avoid the default argument here but we'll want to change this so we don't interact with allowNetworkTraffic here directly anyway, so it doesn't really matter)

gribnoysup

Have one note / nit comment, but otherwise looks good!

gribnoysup · 2025-06-16T11:35:16Z

packages/compass-intercom/src/setup-intercom.ts

-
-  toggleEnableFeedbackPanel(!!enableFeedbackPanel);
+  try {
+    await toggleEnableFeedbackPanel(shouldLoad);


Don't have a good suggestion from the top of my head, but I did find it a bit mind bending that shouldLoad here already takes awaited isIntercomAllowed into account, but then toggleEnableFeedbackPanel immediately fetches it again. I understand the purpose of this, but kinda wish this was maybe somehow less tangled. I don't think it's that big of a deal, but wanted to highlight

It's a valid concern and I had the response cached in a previous commit. I reverted it because it felt a bit premature, but thinking more about it, I do think it makes sense - this is going to be fetched by every render process and we might as well do it once instead of twice.

… is allowed

kraenhansen self-assigned this Jun 3, 2025

kraenhansen added the no release notes Fix or feature not for release notes label Jun 3, 2025

kraenhansen commented Jun 3, 2025

View reviewed changes

kraenhansen force-pushed the kh/intercom-integration-switch branch from 28e787c to 938d4e3 Compare June 6, 2025 13:18

kraenhansen marked this pull request as ready for review June 10, 2025 11:55

kraenhansen added feature flagged PRs labeled with this label will not be included in the release notes of the next release and removed feature flagged PRs labeled with this label will not be included in the release notes of the next release labels Jun 10, 2025

kraenhansen force-pushed the kh/intercom-integration-switch branch from 0f5f80d to 6ab3c4d Compare June 10, 2025 12:39

kraenhansen commented Jun 11, 2025

View reviewed changes

addaleax reviewed Jun 12, 2025

View reviewed changes

kraenhansen requested a review from a team as a code owner June 15, 2025 20:49

kraenhansen force-pushed the kh/intercom-integration-switch branch from cc710c2 to 6441053 Compare June 17, 2025 13:40

gribnoysup approved these changes Jun 17, 2025

View reviewed changes

kraenhansen added 9 commits June 18, 2025 08:53

Add request to the update server to determine if Intercom integration…

e506fcb

… is allowed

Turn toggleEnableFeedbackPanel async and check isIntercomAllowed

d93446c

Remove premature optimization of memoizing integrations response

84a3048

Restore fetch mock correctly

2278f19

Add debug calls

85ed755

Fix tests

c2ad054

Fix lint

5868a43

Use a different domain in proxy tests

bb0847b

Incorporated feedback

1e07960

kraenhansen force-pushed the kh/intercom-integration-switch branch from 6441053 to 1e07960 Compare June 18, 2025 06:53

Cache response from update server

3e165e9

kraenhansen merged commit d5cce0d into main Jun 18, 2025
103 of 105 checks passed

kraenhansen deleted the kh/intercom-integration-switch branch June 18, 2025 11:52

chore(intercom): Add request to the update server to determine if Intercom integration is allowed COMPASS-9371 #6982

chore(intercom): Add request to the update server to determine if Intercom integration is allowed COMPASS-9371 #6982

Uh oh!

Conversation

kraenhansen commented Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Motivation and Context

Open Questions

Dependents

Types of changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Anemy Jun 3, 2025 • 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!

kraenhansen Jun 6, 2025 • 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!

kraenhansen Jun 15, 2025 • 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!

kraenhansen commented Jun 6, 2025

Uh oh!

kraenhansen commented Jun 10, 2025

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!

gribnoysup left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kraenhansen commented Jun 3, 2025 •

edited

Loading

Anemy Jun 3, 2025 •

edited

Loading

kraenhansen Jun 6, 2025 •

edited

Loading

kraenhansen Jun 15, 2025 •

edited

Loading