feat(devtools): Add devtools existence check to pressing the extension button #19287

WayneFerrao · 2024-01-18T22:18:14Z

AB#4525

Description

This PR implements message passing to allow the devtools browser extension to check if the current webpage has devtools and what features it supports.

…dcoded file

jikim-msft · 2024-01-18T22:58:27Z

packages/tools/devtools/devtools-view/src/DevtoolsView.tsx

@@ -239,6 +245,22 @@ export function DevtoolsView(props: DevtoolsViewProps): React.ReactElement {
 		};
 	}, [messageRelay, setSupportedFeatures, telemetryOptInLogger]);

+	React.useEffect(() => {
+		const handleRequest = (event) => {


Is event used in any part of the hook?

Should add return type for handleRequest.

I believe we should include supportedFeatures inside the dependency array, since we want to trigger useEffect whenever there's a change in underlying supportedFeatures value?

Good catch, it isn't. The handler just send the response event. Will fix

packages/tools/devtools/devtools-browser-extension/src/popup/InitializePopupView.ts

Josmithr · 2024-01-30T22:50:44Z

packages/tools/devtools/devtools-browser-extension/src/popup/PopupScript.ts

@@ -19,7 +18,9 @@ import { browser } from "../Globals";
 //   If not, we may want to display an error message with a link to docs explaining how to
 //   use them.

-browser.tabs.query({ active: true, currentWindow: true }, (tab) => {
+const port = browser.runtime.connect({ name: "popup-background-channel" });


Just to make sure I'm understanding correctly: this establishes a connection with the background script specifically? And the name we're providing is supposed to identify this (the caller)?

The name identifies the channel, more than the caller, IIRC. That's why I updated them at some point to be <oneside>-<otherside>-channel.

That said... I wonder if we need to use ports for this use case. browser.runtime.sendMessage() and browser.runtime.onMessage seem like they would do the trick, and I think they might be fine for communication between popup and background, which is more one-off than that between content and background. I think the message sent from popup to background would only trigger listeners set up with browser.runtime.onMessage() in the background script, but if background script did browser.runtime.sendMessage() that would get to all pages of the extension (popup, options, content) so maybe we'd need to make sure that content ignores this one.

Or actually, if popup used sendMessage(), background could use sendResponse() on the message it gets to respond synchronously (an async option is available too, all documented in onMessage()) directly to popup, and I guess we don't need to have special considerations in content in that case.

packages/tools/devtools/devtools-browser-extension/src/popup/PopupScript.ts

msfluid-bot · 2024-02-02T23:44:50Z

Could not find a usable baseline build with search starting at CI 40e6559

Generated by 🚫 dangerJS against cbd0ede

alexvy86

I think it's almost there! A few comments below.

packages/tools/devtools/devtools-browser-extension/src/popup/InitializePopupView.ts

packages/tools/devtools/devtools-browser-extension/src/popup/PopupView.tsx

Josmithr · 2024-02-12T18:20:36Z

packages/tools/devtools/devtools-browser-extension/src/popup/PopupScript.ts

+ */
+export async function initializePopupView(target: HTMLElement, tabId: number): Promise<void> {
+	const backgroundServiceConnection = await BackgroundConnection.Initialize({
+		// TODO: devtools-panel-specific source


I would recommend we make this change before merging - it will help us determine whether requests are coming from the popup or the devtools view when debugging.

We just need to be careful to ensure existing flows continue to work (manual testing + auditing code to check for any places checking for the specific existing source here).

packages/tools/devtools/devtools-browser-extension/src/popup/PopupScript.ts

packages/tools/devtools/devtools-browser-extension/src/popup/PopupView.tsx

Josmithr · 2024-02-12T18:26:54Z

packages/tools/devtools/devtools-browser-extension/src/popup/PopupView.tsx

+			)}
+			{foundDevtools === false && (
+				<div>
+					Devtools not found. Visit the documentation{" "}


I would vote for going into more detail here. E.g. "Fluid Devtools were not found running in the current tab. For details on how to enable the Devtools in your Fluid application, please refer to our documentation..."

@kashms interested in your thoughts on exact messaging for these. / If we should include any graphics or anything.

packages/tools/devtools/devtools-browser-extension/src/popup/PopupView.tsx

packages/tools/devtools/devtools-browser-extension/src/popup/PopupScript.ts

Josmithr · 2024-02-12T18:29:56Z

packages/tools/devtools/devtools-browser-extension/src/popup/PopupScript.ts

@@ -19,7 +23,26 @@ import { browser } from "../Globals";
 //   If not, we may want to display an error message with a link to docs explaining how to
 //   use them.

-browser.tabs.query({ active: true, currentWindow: true }, (tab) => {
+// TODO: double check that tab selection can't change while popup is being displayed.


We should verify this before merging. If it is possible for the tab selection to change while the popup is up, we should file a follow-up work item to handle tab selection changes gracefully, and update the TODO comment. If it isn't possible, we can just remove this comment :)

@WayneFerrao I take it that it's not possible for the tab selection to change while the popup is being displayed? If so, let's leave a note to that effect as a comment, in case a future reader is wondering about this.

Josmithr

This is looking great! Left a few more comments, but from an infrastructure standpoint this looks perfect to me.

Josmithr · 2024-02-14T18:24:11Z

Would you mind adding some screenshots to the PR description?

WayneFerrao and others added 13 commits December 7, 2023 12:33

WIP: Updated type test to check for and use alpha package

e69be3a

Updated functions and test generation to allow for alpha files

101c660

Removed unneeded alpha checks

6479a75

Update type test config and logic to check previous package.json

47ba9a9

Merge branch 'main' into updateTypeTestsAlpha

8307910

updated rollup file name

f611dce

Merge branch 'main' into updateTypeTestsAlpha

8319553

Updated logic to point to rollup file in api extractor instead of har…

d43112c

…dcoded file

Merge branch 'main' into updateTypeTestsAlpha

1723d54

Merge branch 'main' into updateTypeTestsAlpha

c7b4298

fixed linting

95acd1d

Added PopupView and attempted window message passing

ce6e63a

Implemented round trip message passing from extension to webpage

a3677f2

WayneFerrao requested review from a team as code owners January 18, 2024 22:18

github-actions bot added base: main PRs targeted against main branch area: build Build related issues labels Jan 18, 2024

WayneFerrao force-pushed the devtoolsExistenceCheck branch from f0acd3a to 39a5125 Compare January 18, 2024 22:42

Revert irrelevant changes

8f688a6

WayneFerrao force-pushed the devtoolsExistenceCheck branch from 39a5125 to 8f688a6 Compare January 18, 2024 22:43

github-actions bot removed the area: build Build related issues label Jan 18, 2024

Merge branch 'main' into devtoolsExistenceCheck

8faa681

jikim-msft reviewed Jan 18, 2024

View reviewed changes

WayneFerrao added 3 commits January 18, 2024 18:03

Removed unneeded param and added return type

13a40e1

PR fix

6fa1d80

Merge branch 'main' into devtoolsExistenceCheck

87b75cb

Josmithr reviewed Jan 30, 2024

View reviewed changes

packages/tools/devtools/devtools-browser-extension/src/popup/InitializePopupView.ts Outdated Show resolved Hide resolved

Josmithr reviewed Jan 30, 2024

View reviewed changes

packages/tools/devtools/devtools-browser-extension/src/popup/PopupScript.ts Outdated Show resolved Hide resolved

Josmithr reviewed Jan 30, 2024

View reviewed changes

packages/tools/devtools/devtools-browser-extension/src/popup/PopupScript.ts Outdated Show resolved Hide resolved

Revert unneeded changes

2f415f2

WayneFerrao force-pushed the devtoolsExistenceCheck branch from 5544ce0 to 2f415f2 Compare February 2, 2024 22:38

WayneFerrao added 3 commits February 2, 2024 17:44

Revert old content/background script changes

dc09079

Code cleanup

33ae026

Updated Devtools found message

8cad022

WayneFerrao force-pushed the devtoolsExistenceCheck branch from bf6abf1 to 8cad022 Compare February 2, 2024 23:11

Merge branch 'main' into devtoolsExistenceCheck

3fc286f

WayneFerrao requested a review from a team February 5, 2024 21:06

alexvy86 reviewed Feb 6, 2024

View reviewed changes

WayneFerrao requested a review from a team as a code owner February 6, 2024 20:04

PR fixes, moved initialize function to Popupview

bd8c85c

WayneFerrao force-pushed the devtoolsExistenceCheck branch from 1e78359 to bd8c85c Compare February 6, 2024 20:17

WayneFerrao added 2 commits February 6, 2024 16:36

Updated timer logic to be in same useEffect

611b20e

Merge branch 'main' into devtoolsExistenceCheck

d680c12

alexvy86 reviewed Feb 7, 2024

View reviewed changes

packages/tools/devtools/devtools-browser-extension/src/popup/PopupView.tsx Outdated Show resolved Hide resolved

alexvy86 reviewed Feb 7, 2024

View reviewed changes

packages/tools/devtools/devtools-browser-extension/src/popup/PopupView.tsx Outdated Show resolved Hide resolved

Cleaned up code and removed unnecessary dependency in useEffect

a38b374

Josmithr reviewed Feb 12, 2024

View reviewed changes

packages/tools/devtools/devtools-browser-extension/src/popup/PopupScript.ts Outdated Show resolved Hide resolved

Josmithr reviewed Feb 12, 2024

View reviewed changes

packages/tools/devtools/devtools-browser-extension/src/popup/PopupView.tsx Outdated Show resolved Hide resolved

Josmithr reviewed Feb 12, 2024

View reviewed changes

packages/tools/devtools/devtools-browser-extension/src/popup/PopupView.tsx Outdated Show resolved Hide resolved

Josmithr reviewed Feb 12, 2024

View reviewed changes

packages/tools/devtools/devtools-browser-extension/src/popup/PopupScript.ts Outdated Show resolved Hide resolved

Josmithr reviewed Feb 12, 2024

View reviewed changes

WayneFerrao added 2 commits February 12, 2024 17:10

Verified todos and updated text

baf5fcb

Merge branch 'main' into devtoolsExistenceCheck

88960a0

updated popup box width

cbd0ede

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(devtools): Add devtools existence check to pressing the extension button #19287

feat(devtools): Add devtools existence check to pressing the extension button #19287

WayneFerrao commented Jan 18, 2024 •

edited

jikim-msft Jan 18, 2024 •

edited

WayneFerrao Jan 18, 2024

Josmithr Jan 30, 2024

alexvy86 Jan 31, 2024 •

edited

msfluid-bot commented Feb 2, 2024 •

edited

alexvy86 left a comment

Josmithr Feb 12, 2024

Josmithr Feb 12, 2024

Josmithr Feb 12, 2024

Josmithr Feb 12, 2024

Josmithr Feb 14, 2024

Josmithr left a comment

Josmithr commented Feb 14, 2024

feat(devtools): Add devtools existence check to pressing the extension button #19287

Are you sure you want to change the base?

feat(devtools): Add devtools existence check to pressing the extension button #19287

Conversation

WayneFerrao commented Jan 18, 2024 • edited

Description

jikim-msft Jan 18, 2024 • edited

Choose a reason for hiding this comment

WayneFerrao Jan 18, 2024

Choose a reason for hiding this comment

Josmithr Jan 30, 2024

Choose a reason for hiding this comment

alexvy86 Jan 31, 2024 • edited

Choose a reason for hiding this comment

msfluid-bot commented Feb 2, 2024 • edited

alexvy86 left a comment

Choose a reason for hiding this comment

Josmithr Feb 12, 2024

Choose a reason for hiding this comment

Josmithr Feb 12, 2024

Choose a reason for hiding this comment

Josmithr Feb 12, 2024

Choose a reason for hiding this comment

Josmithr Feb 12, 2024

Choose a reason for hiding this comment

Josmithr Feb 14, 2024

Choose a reason for hiding this comment

Josmithr left a comment

Choose a reason for hiding this comment

Josmithr commented Feb 14, 2024

WayneFerrao commented Jan 18, 2024 •

edited

jikim-msft Jan 18, 2024 •

edited

alexvy86 Jan 31, 2024 •

edited

msfluid-bot commented Feb 2, 2024 •

edited