web: Add Test connection button and API call to code host by indradhanush · Pull Request #45972 · sourcegraph/sourcegraph-public-snapshot

indradhanush · 2022-12-26T14:19:29Z

UI for #44683.

Test plan

Tested locally

Check connection unavailable

Check connection available

Check connection successful

Check connection failed

Check connection errored (application error)

Test button in action 🎥 for success, failure and error modes

Builds should pass

App preview:

Web

Check out the client app preview documentation to learn more.

sg-e2e-regression-test-bob · 2022-12-26T14:26:38Z

Bundle size report 📦

Initial size	Total size	Async size	Modules
0.01% (+0.38 kb)	0.03% (+4.48 kb)	0.04% (+4.10 kb)	0.00% (0)

Look at the Statoscope report for a full comparison between the commits f502511 and 821918c or learn more.

Open explanation

Initial size is the size of the initial bundle (the one that is loaded when you open the page)
Total size is the size of the initial bundle + all the async loaded chunks
Async size is the size of all the async loaded chunks
Modules is the number of modules in the initial bundle

Step 1 of many. Need to tie down the response to the button to show changes.

Needs cleanup, but first lunch 🍔

indradhanush · 2022-12-29T10:26:08Z

        updatedAt: '2021-03-15T19:39:11Z',
        createdAt: '2021-03-15T19:39:11Z',
        webhookURL: null,
+        hasConnectionCheck: true,


This change brought to you by yarn build-ts errors.

indradhanush · 2022-12-29T10:26:54Z

                id: 'service1',
                kind: ExternalServiceKind.GITHUB,
-                displayName: 'GitHub.com',
+                displayName: 'GitHub #1',


Small fix: Fixed a warning for both the displayName being same in the storybook.

0xnmn · 2022-12-29T10:38:22Z

+                                    <span className={classNames('text-primary')}>
+                                        <LoadingSpinner /> Checking connection...
+                                    </span>
+                                )}


Do this rather(same for other places):

const checkConnectionNode = checkConnectionData?.node const isCodeHostReachable = node?.__typename === 'ExternalService' && node?.checkConnection?.__typename === 'ExternalServiceAvailable' && node?.checkConnection?.lastCheckedAt; {isCodeHostReachable && ( <span className="text-success"> <Icon aria-hidden={true} svgPath={mdiCheckCircle} /> Code host is reachable. </span> )}

sashaostrikov · 2022-12-29T12:01:30Z

+        { called: checkConnectionCalled, loading: checkConnectionLoading, data: checkConnectionData },
+    ] = useExternalServiceCheckConnectionByIdLazyQuery(node.id)
+
+    const [checkConnectionError, setCheckConnectionError] = useState<boolean | Error>(false)


I guess the type of state can be simplified to just Error and initially set to undefined.
(Or you might need Error | undefined, I'm not 100% sure 😁)

Hmm. Probably. But we're doing something similar already for isDeleting a little above this code. Maybe we keep them the same to have a recognizable pattern in this part of the code? What do you think?

useLazyQuery should return error on its own and you don't need to save error state separately.

You can do const [doCheckConnection, { loading, data, error }] = useExternalServiceCheckConnectionByIdLazyQuery(node.id)

deleteExternalService is legacy and not using the react hooks; that is why the error is handled separately there.

Read: https://www.apollographql.com/docs/react/api/react/hooks/#error

sashaostrikov · 2022-12-29T12:12:34Z

                                )}
                                {node.nextSyncAt === null && <>No next sync scheduled.</>}
+                                <br />
+                                {isErrorLike(checkConnectionError) && (


I think this can be simplified to !checkConnectionError. Because currently it is either false or Error which is isErrorLike

probably, loading check should be added here as well

I think this can be simplified to !checkConnectionError. Because currently it is either false or Error which is isErrorLike

I think the isErrorLike is a stronger check here in case there's ever a bug and checkConnectionError is true, then we will end up displaying an error message with true in it. I checked in other places and it looks like isErrorLike is used in such cases. I'd vote to keep it as is.

+1 on moving the loading check to be the first. Fixed that.

don't believe this can happen, we need to trust our code :)
but still it's your call

don't believe this can happen, we need to trust our code :)

Yeah I trust the code now. But I'm thinking that the current check is stronger for any potential future bugs if anyone / me end up setting true as a value by mistake only because its technically allowed by the type here. Just defensive programming I guess. 😛

Since it's not too complicated in its current form and also is used quite widely in our code, keeping it is not the worst idea. 😀

Improve code organisation and variable naming.

sashaostrikov

LGTM!
But let's wait for @thenamankumar as well.

0xnmn · 2022-12-29T17:27:21Z

+
+    const [checkConnectionError, setCheckConnectionError] = useState<boolean | Error>(false)
+
+    const onTestConnection = useCallback<React.MouseEventHandler>(async () => {


use error from useLazyQuery

Fixed in the latest commit 98e18be.

You can directly use doCheckConnection. No need to wrap with useCallback.

0xnmn

https://github.com/sourcegraph/sourcegraph/pull/45972/files#r1059071884

No need for extra state management for error handling. 🎉

0xnmn · 2022-12-30T06:52:19Z

+
+    const [checkConnectionError, setCheckConnectionError] = useState<boolean | Error>(false)
+
+    const onTestConnection = useCallback<React.MouseEventHandler>(async () => {


You can directly use doCheckConnection. No need to wrap with useCallback.

mrnugget · 2023-01-02T10:15:47Z

                </div>
                <div className="flex-shrink-0 ml-3">
+                    <Tooltip
+                        content={node.hasConnectionCheck ? 'Test code host connection' : 'Connection check unavailable'}


I'd change Test code host connection to Test whether code host is reachable from Sourcegraph

Created a PR: https://github.com/sourcegraph/sourcegraph/pull/46029

Tweaked the suggested message slightly to make it a little shorter.

mrnugget · 2023-01-02T10:22:07Z

+                            className="test-connection-external-service-button"
+                            variant="secondary"
+                            onClick={() => doCheckConnection()}
+                            disabled={!node.hasConnectionCheck || loading}


(man, GitHub lost my comment, so here it is again)

I think we should just hide the button if !node.hasConnectionCheck. A disabled button suggests there's a way to enable it. In this case though, we don't support the connection check and a Sourcegraph update is required to get it enabled once we add support, meaning: nothing the user can do.

(man, GitHub lost my comment, so here it is again)

Have been seeing some reliability issues with GitHub myself this week. 🙁

I get how a user might thinkg that the disabled button suggests that there's a way to enabled it. But with the suggested change, it looks inconsistent to me when one entry doesn't have that button. Besides we do indicate that the connection check is unavailable on the disabled button. Think that should be okay?

If it looks all right to you, I can create the PR.

I think that's alright. (Ideally and further down the road we create a "details" page for code hosts where we can put this)

cla-bot Bot added the cla-signed label Dec 26, 2022

web: Add Test connection button and API call to code host

5c677c2

Step 1 of many. Need to tie down the response to the button to show changes.

indradhanush force-pushed the ig/test-connection-button branch from 35e2008 to 5c677c2 Compare December 27, 2022 03:30

indradhanush added 14 commits December 27, 2022 15:23

WIP: Add loading spinner

7ac316a

WIP Save workig spinner and success / failure message

f7ff902

Needs cleanup, but first lunch 🍔

Remove unused code and make success greenn and failure red

b1d0b6e

WIP

be49d80

Load spinner in status message

57dc2fd

WIP

fa85c5e

client/web/src: sg lint -fix format

c01ceb3

Handle potential error and use ErrorMessage

0377776

client/web: UseQueryTuple for useLazyQuery

5f9dd0a

sg lint -fix format

ec50662

Add check for checkConnectionData.node.__typename

61318cd

Update fixture, error caught by yarn build-ts

6fd420e

Fix storybook error from yarn build-ts

e5c0ef2

JS lint fixes

cceaaf2

indradhanush marked this pull request as ready for review December 29, 2022 10:23

indradhanush requested review from a team and 0xnmn December 29, 2022 10:24

indradhanush added site-admin Site admin experience site-admin-ux Issues related to site-admin UX: bugs, papercuts, design, ... labels Dec 29, 2022

indradhanush commented Dec 29, 2022

View reviewed changes

0xnmn suggested changes Dec 29, 2022

View reviewed changes

🧹 externalServices: Cleaner code - store state in const

0ae89d3

indradhanush requested a review from 0xnmn December 29, 2022 11:20

sashaostrikov reviewed Dec 29, 2022

View reviewed changes

externalServices: Simplify redundant checks

d32d4e9

Improve code organisation and variable naming.

indradhanush requested review from a team and sashaostrikov December 29, 2022 13:09

externalServices: Remove unused variable called

56b039c

sashaostrikov approved these changes Dec 29, 2022

View reviewed changes

0xnmn reviewed Dec 29, 2022

View reviewed changes

0xnmn suggested changes Dec 29, 2022

View reviewed changes

externalServices: Use error from useLazyQuery

98e18be

No need for extra state management for error handling. 🎉

indradhanush requested a review from 0xnmn December 30, 2022 03:30

0xnmn suggested changes Dec 30, 2022

View reviewed changes

externalServices: Directly call doCheckConnection

d15e9cb

0xnmn approved these changes Dec 30, 2022

View reviewed changes

indradhanush enabled auto-merge (squash) December 30, 2022 07:41

indradhanush disabled auto-merge December 30, 2022 07:43

externalServices: Use anonymouse func to satisfy type check

f502511

indradhanush merged commit 23a42cf into main Dec 30, 2022

indradhanush deleted the ig/test-connection-button branch December 30, 2022 10:52

indradhanush mentioned this pull request Jan 2, 2023

graphqlbackend: Simplify external service availability API #46024

Closed

mrnugget reviewed Jan 2, 2023

View reviewed changes

indradhanush mentioned this pull request Jan 2, 2023

externalServices: Better tooltip description of test button #46029

Merged


		const [checkConnectionError, setCheckConnectionError] = useState<boolean \| Error>(false)

		const onTestConnection = useCallback<React.MouseEventHandler>(async () => {

Conversation

indradhanush commented Dec 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test plan

App preview:

Uh oh!

sg-e2e-regression-test-bob commented Dec 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bundle size report 📦

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!

Choose a reason for hiding this comment

Uh oh!

0xnmn Dec 29, 2022 • 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!

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!

Uh oh!

Uh oh!

sashaostrikov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

indradhanush Dec 30, 2022 • 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!

0xnmn 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!

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!

indradhanush Jan 2, 2023 • 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!

indradhanush Jan 2, 2023 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

indradhanush commented Dec 26, 2022 •

edited

Loading

sg-e2e-regression-test-bob commented Dec 26, 2022 •

edited

Loading

0xnmn Dec 29, 2022 •

edited

Loading

indradhanush Dec 30, 2022 •

edited

Loading

indradhanush Jan 2, 2023 •

edited

Loading

indradhanush Jan 2, 2023 •

edited

Loading