Rework reconnect logic, add "Port could not be opened" modal #1513

phryneas · 2024-09-09T14:18:48Z

Also see the accompanying apollographql/vscode-graphql#197

phryneas · 2024-09-11T08:20:51Z

src/application/App.tsx

-        onClose={() => send({ type: "closeModal" })}
-        onRetry={() => send({ type: "retry" })}
-      />
+      <Modals />


All modals live in that file now.

Can we name this something like ErrorModals? Ideally we have some namespace that describes that these are the modals opened when the clients can't connect in some way.

Otherwise it looks a little bit weird to have SettingsModal right next to Modals 😂.

phryneas · 2024-09-11T08:30:46Z

src/application/App.tsx

      <BannerAlert />
      <Tabs
        value={selected}
        onChange={(screen) => currentScreen(screen)}
        className="flex flex-col h-screen bg-primary dark:bg-primary-dark"
      >
        <div className="flex items-center border-b border-b-primary dark:border-b-primary-dark gap-4 px-4">
-          <a
+          <ExternalLink


<a tags in VSCode cannot open new windows, so I need a component for that. The base implementation really just renders an a tag, and we have a .vscode.tsx version of that.

phryneas · 2024-09-11T08:32:10Z

src/application/components/GitHubIssueLink.tsx

@@ -21,7 +22,7 @@ export const SECTIONS = {
 <!-- Please provide the version of \`@apollo/client\` you are using. -->
 `,
  devtoolsVersion: `### Apollo Client Devtools version
-${VERSION}
+${VERSION} (${__IS_FIREFOX__ ? "Firefox" : __IS_VSCODE__ ? "VSCode" : "Chrome"})


Some additional info can be very helpful here to distinguish the different builds in bug reports.

phryneas · 2024-09-11T08:32:47Z

src/application/components/Modal/Modal.tsx

@@ -18,15 +18,17 @@ interface ModalProps {
  className?: string;
  children: ReactNode;
  open: boolean;
-  onClose: (value: boolean) => void;
+  onClose?: (value: boolean) => void;


Making onClose optional since most of our modals cannot be closed.

phryneas · 2024-09-11T08:41:39Z

src/application/components/Modals/ClientNotFoundModal.tsx

  open: boolean;
-  onClose: () => void;
  onRetry: () => void;
 }


This modal cannot be closed anymore - before, closing it would leave the user with a somewhat defunct UI, so it never made much sense to me. Going this step massively simplifies logic.

phryneas · 2024-09-11T08:49:49Z

src/application/machines/devtoolsMachine.tsx

+          invoke: {
+            id: "reconnect",
+            src: "reconnect",
+          },


This invokes the child machine while in this state and stops it as soon as this state is left.

phryneas · 2024-09-11T08:51:43Z

src/application/machines/devtoolsMachine.tsx

    },
  },
 });
+
+export const DevToolsMachineContext = createContext<DevToolsActor | null>(null);


We now use Context to pass this down into the React app - but not the createContext from xState, as that would create the actor from the machine and we want to create the actor outside of React (the Actor renders React).

phryneas · 2024-09-11T08:53:23Z

src/application/machines/reconnectMachine.tsx

+        "reconnect.retry": "retrying",
+      },
+      entry: ["notifyNotFound"],
+    },


Previously, this also had a timedout state, but we never triggered it from anywhere.

I'm glad you got rid of it. It was a relic from the days before we had the client send a register event. Devtools used to ping the content scripts, so the timedout event was used if devtools couldn't communicate with the content scripts anymore, but that is largely a thing of the past.

phryneas · 2024-09-11T08:54:26Z

src/application/machines/reconnectMachine.tsx

+        },
+      },
+    },
+    notFound: {


The modal is now shown while the machine is in this state. If we wanted to show the modal in more states that just one, we could look into tag.

phryneas · 2024-09-11T08:55:23Z

src/application/machines/reconnectMachine.vscode.tsx

+export const reconnectMachine = setup({
+  delays: { connectTimeout: 500 },
+}).createMachine({


In VSCode, we want to show this from the start, as it has valuable information.
We will wait for a very short amount of time though, in case something immediately connects - that way we avoid flickering of the modal.

adjustments for apollographql/apollo-client-devtools#1513

pkg-pr-new · 2024-09-20T15:15:14Z

npm i https://pkg.pr.new/apollographql/apollo-client-devtools@1513

npm i https://pkg.pr.new/apollographql/apollo-client-devtools/@apollo/client-devtools-vscode@1513

commit: 06c817b

jerelmiller · 2024-12-02T23:11:13Z

src/application/components/ExternalLink.tsx

+  HTMLAnchorElement,
+  ComponentProps<"a"> & { href: string }
+>((props, ref) => {
+  return <a {...props} ref={ref} />;


Since we are moving to this, let's add target="_blank" rel="noopener noreferrer" as props here as well since external links on the web side don't open unless they are in a new tab.

jerelmiller · 2024-12-02T23:20:31Z

src/application/components/GitHubReleaseHoverCard.tsx

              target="_blank"
              rel="noreferrer"


Suggested change

target="_blank"

rel="noreferrer"

Can we remove these props once the browser ExternalLink is updated to add these props directly?

relativeci · 2024-12-04T12:35:01Z

#1061 Bundle Size — 1.63MiB (+0.68%).

06c817b(current) vs 31394f0 main#1051(baseline)

Warning

Bundle contains 13 duplicate packages – View duplicate packages

Bundle metrics

4 changes

1 regression

	Current #1061	Baseline #1051
Initial JS	`1.49MiB`(`+0.19%`)	`1.49MiB`
Initial CSS	`0B`	`0B`
Cache Invalidation	`88.78%`	`0%`
Chunks	`5`	`5`
Assets	`157`(`+6.8%`)	`147`
Modules	`1228`(`+0.33%`)	`1224`
Duplicate Modules	`45`	`45`
Duplicate Code	`3.12%`	`3.12%`
Packages	`183`	`183`
Duplicate Packages	`10`	`10`

Bundle size by type

2 changes

2 regressions

	Current #1061	Baseline #1051
JS	`1.49MiB` (`+0.19%`)	`1.49MiB`
Other	`100.55KiB` (`+8.92%`)	`92.32KiB`
IMG	`35.85KiB`	`35.85KiB`
HTML	`857B`	`857B`

Bundle analysis report Branch pr/port-info Project dashboard

^{Generated by RelativeCI Documentation Report issue}

phryneas · 2024-12-04T12:57:36Z

Okay, I went over that - re-review please :)

adjustments for apollographql/apollo-client-devtools#1513

jerelmiller · 2024-12-06T17:02:56Z

src/application/machines/devtoolsMachine.tsx

+      actions: sendTo("reconnect", ({ event }) => event),
+    },
+    // on an `emit.*` event, `emit` that event so it can be subscribed to from the app
+    "emit.*": {


I'm curious about this one. I searched the changes for anything that listens to these emitted events, but I didn't see anything. Am I missing something obvious?

If not used, I'm curious do you still want to keep the emitted events?

Before https://github.com/apollographql/apollo-client-devtools/pull/1536/files, this would allow us to call refetch as a reaction to that event.

It's not like we need this anymore right now, but it is a good blueprint we can build on if we need something like this in the future, and it doesn't hurt, so I'd keep it in for now.

jerelmiller · 2024-12-06T20:45:28Z

src/application/components/Modals/PortNotOpenModal.vscode.tsx

+const StartCommandLabel = "Apollo: Start Apollo Client DevTools Server";
+const StartCommand = "apollographql/startDevToolsServer";
+const StopCommandLabel = "Apollo: Stop Apollo Client DevTools Server";
+const PortSetting = "apollographql.devTools.serverPort";


Mind switching these either to SCREAMING_SNAKE_CASE or camelCase? We don't use PascalCase for anything but components, classes or types so would be great to keep this consistent 🙂.

4679bd9 ✅

jerelmiller · 2024-12-06T20:49:33Z

src/application/components/VSCode/VSCodeCommandButton.tsx

I go back on forth on the VSCode folder. While I agree that it does convey its meant for VSCode, we've been using the .vscode extension here for that. I tend to prefer that since it remains consistent for every vscode component.

If you make this change, perhaps in the file its imported, you just import with the .vscode extension so you don't have to create a browser version?

// PortNotOpenModal.tsx import { VSCodeCommandButton } from './VSCodeCommandButton.vscode';

jerelmiller · 2024-12-06T20:50:17Z

src/application/components/VSCode/VSCodeCommandButton.tsx

+import { Button, type ButtonProps } from "../Button";
+import { forwardRef } from "react";
+
+export const VSCodeCommandButton = forwardRef<


Now we really gotta update to React 19 😆

jerelmiller · 2024-12-06T20:51:15Z

src/application/components/Modals/PortNotOpenModal.vscode.tsx

+                  {StartCommandLabel}
+                </VSCodeCommandButton>
+                <VSCodeSettingButton settingsKey={PortSetting}>
+                  Open Settings


Suggested change

Open Settings

Open settings

Studio uses sentence case for everything, so we should do the same here 🙂

jerelmiller · 2024-12-06T20:52:58Z

src/application/components/VSCode/VSCodeSettingButton.tsx

+    <Button
+      variant="primary"
+      size="sm"
+      icon={<IconRun />}


Let's use IconSettings which looks like this:

jerelmiller · 2024-12-06T20:55:46Z

src/application/components/VSCode/VSCodeSettingButton.tsx

+        getPanelActor(window).send({
+          type: "vscode:executeCommand",
+          command: "workbench.action.openSettings",
+          arguments: [settingsKey],


Could we use args as the key here? I know this is technically fine, but makes it easier to distinguish that its not arguments available inside a function when looking at it from a glance. I think this was used on the vscode side?

src/application/components/VSCode/VSCodeSettingButton.tsx

jerelmiller · 2024-12-06T21:00:44Z

src/application/machines/reconnectMachine.tsx

+        "reconnect.retry": "retrying",
+      },
+      entry: ["notifyNotFound"],
+    },


I'm glad you got rid of it. It was a relic from the days before we had the client send a register event. Devtools used to ping the content scripts, so the timedout event was used if devtools couldn't communicate with the content scripts anymore, but that is largely a thing of the past.

jerelmiller · 2024-12-06T21:07:53Z

src/application/App.tsx

-        onClose={() => send({ type: "closeModal" })}
-        onRetry={() => send({ type: "retry" })}
-      />
+      <Modals />


Can we name this something like ErrorModals? Ideally we have some namespace that describes that these are the modals opened when the clients can't connect in some way.

Otherwise it looks a little bit weird to have SettingsModal right next to Modals 😂.

jerelmiller

Went over this in person.

Only feedback I have left is to rename that Modals folder (#1513 (comment)), but I'll trust you'll do that.

Looking forward to getting this out!

* Adjustments to VSCode <-> DevTools communication adjustments for apollographql/apollo-client-devtools#1513 * use CI devtools build * feedback from code review * second adjustment * adjust CI to GH runner changes

phryneas force-pushed the pr/vscode-connect-info branch from f79588c to f68639f Compare September 10, 2024 08:16

phryneas force-pushed the pr/port-info branch from 202b490 to 2648fde Compare September 10, 2024 08:18

phryneas commented Sep 11, 2024

View reviewed changes

phryneas marked this pull request as ready for review September 11, 2024 08:56

phryneas requested a review from a team as a code owner September 11, 2024 08:56

phryneas added a commit to apollographql/vscode-graphql that referenced this pull request Sep 11, 2024

Adjustments to VSCode <-> DevTools communication

9d0d030

adjustments for apollographql/apollo-client-devtools#1513

phryneas mentioned this pull request Sep 11, 2024

Adjustments to VSCode <-> DevTools communication apollographql/vscode-graphql#197

Merged

phryneas force-pushed the pr/vscode-connect-info branch from f68639f to 961a1ae Compare September 20, 2024 12:46

phryneas force-pushed the pr/port-info branch from 405da4b to 6975f63 Compare September 20, 2024 12:55

phryneas added a commit to apollographql/vscode-graphql that referenced this pull request Sep 20, 2024

Adjustments to VSCode <-> DevTools communication

83d67b2

adjustments for apollographql/apollo-client-devtools#1513

phryneas added a commit to apollographql/vscode-graphql that referenced this pull request Sep 20, 2024

use CI build from apollographql/apollo-client-devtools#1513

f2dc476

jerelmiller reviewed Dec 2, 2024

View reviewed changes

Base automatically changed from pr/vscode-connect-info to main December 4, 2024 12:10

phryneas added 15 commits December 4, 2024 13:24

move initialization into state machine

7b425dc

use parallel states

57d6d0b

move reconnect logic into invoked actor

dd0ade5

modals

c14e3e6

update semgrepignore

2af9d25

split machine into files

55d3d49

make dialogs non-closable

cf6d324

update tests

2f5c1ec

fix emitting of store.didReset

d969eaa

add debug logic

a97a5c8

progress

25426d7

adjustments

8033510

use ExternalLink everywhere

aec4123

more readable logic around "only one modal open"

359a4d6

minor cleanup

1978995

move target="_blank" rel="noopener noreferrer" into ExternalLink

0550206

phryneas force-pushed the pr/port-info branch from 10ac491 to 0550206 Compare December 4, 2024 12:33

update more noopener usages

a9505f7

phryneas requested a review from jerelmiller December 4, 2024 12:57

phryneas added a commit to apollographql/vscode-graphql that referenced this pull request Dec 6, 2024

Adjustments to VSCode <-> DevTools communication

3700b50

adjustments for apollographql/apollo-client-devtools#1513

jerelmiller reviewed Dec 6, 2024

View reviewed changes

phryneas added 3 commits December 9, 2024 16:47

suggestion from code review

4679bd9

feedback from code review

623eb1f

add .vscode extension

33860bd

jerelmiller approved these changes Dec 9, 2024

View reviewed changes

phryneas added 3 commits December 10, 2024 15:12

Modals -> ErrorModals

9398e4c

Merge remote-tracking branch 'origin/main' into pr/port-info

7ab936a

changesets

06c817b

phryneas merged commit f6305df into main Dec 10, 2024
11 of 12 checks passed

phryneas deleted the pr/port-info branch December 10, 2024 14:30

github-actions bot mentioned this pull request Dec 10, 2024

Version Packages #1592

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework reconnect logic, add "Port could not be opened" modal #1513

Rework reconnect logic, add "Port could not be opened" modal #1513

phryneas commented Sep 9, 2024 •

edited

Loading

phryneas Sep 11, 2024

jerelmiller Dec 6, 2024

phryneas Sep 11, 2024

phryneas Sep 11, 2024

phryneas Sep 11, 2024

phryneas Sep 11, 2024

phryneas Sep 11, 2024

phryneas Sep 11, 2024

phryneas Sep 11, 2024

jerelmiller Dec 6, 2024

phryneas Sep 11, 2024

phryneas Sep 11, 2024

pkg-pr-new bot commented Sep 20, 2024 •

edited

Loading

jerelmiller Dec 2, 2024

jerelmiller Dec 2, 2024

relativeci bot commented Dec 4, 2024 •

edited

Loading

phryneas commented Dec 4, 2024

jerelmiller Dec 6, 2024

phryneas Dec 9, 2024

jerelmiller Dec 6, 2024

phryneas Dec 9, 2024

jerelmiller Dec 6, 2024

jerelmiller Dec 6, 2024

jerelmiller Dec 6, 2024

jerelmiller Dec 6, 2024

jerelmiller Dec 6, 2024

jerelmiller Dec 6, 2024

jerelmiller Dec 6, 2024

jerelmiller left a comment

Rework reconnect logic, add "Port could not be opened" modal #1513

Rework reconnect logic, add "Port could not be opened" modal #1513

Conversation

phryneas commented Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkg-pr-new bot commented Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

relativeci bot commented Dec 4, 2024 • edited Loading

#1061 Bundle Size — 1.63MiB (+0.68%).

phryneas commented Dec 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerelmiller left a comment

Choose a reason for hiding this comment

phryneas commented Sep 9, 2024 •

edited

Loading

pkg-pr-new bot commented Sep 20, 2024 •

edited

Loading

relativeci bot commented Dec 4, 2024 •

edited

Loading