fix: command palette

[slimsag]

A regression (#262) which broke the command palette was introduced (but not published). This fixes that regression.

• The regression was introduced by @sqs's change to e-c-c here: sourcegraph/extensions-client-common@19df39f
• The dependency was updated in this repository by @chrismwendt here: 8616d66

The issue is that we are using a @shopify/react-shortcuts package to declare keyboard shortcuts in e-c-c. The package registers keyboard shortcuts via a <Shortcut> component which requires that a <ShortcutProvider> component be somewhere above it in the React rendering tree, in order to pass down some React context.

In the Sourcegraph web application, we provide such an implementation: https://sourcegraph.sgdev.org/github.com/sourcegraph/sourcegraph/-/blob/src/SourcegraphWebApp.tsx#L259:14 Here in the browser extension, however, we do not -- and this causes any <Shortcut> component to break due to missing React context data. If we were to publish the browser extension today, the Sourcegraph Extension command palette would be broken outright (clicking on it causes it to disappear).

Fixing the issue is tricky: @shopify/react-shortcuts's ShortcutProvider component is designed with the assumption that there will only ever be a single React component rendering the entire application. It uses this assumption to only register a single document keyboard event handler, otherwise it will install multiple global event handlers to the document that conflict with one another. In most applications, this assumption holds true, but in our situation here with the browser extension this is not true as we inject independent React views into several different locations on the page and multiple expect to be able to add <Shortcut>s.

Fixing this becomes more tricky because @shopify/react-shortcuts does not expose the Provider part of the React.createContext result -- meaning it is impossible for us to implement our own ShortcutProvider that fixes this issue. I've had to temporarily fork react-shortcuts to expose this, and if all looks good I will create a PR upstream for this: https://github.com/slimsag/quilt/tree/publish-provider In the meantime, we will have to use my fork everywhere (if we don't, React context will not grab the right data and it'll e.g. break the command palette on Sourcegraph.com).

This does not actually cause us to register shortcuts in the browser extension for the command palette -- but we certainly can now after this change.

See also:

• fix: use @slimsag/react-shortcuts extensions-client-common#60
• use @slimsag/react-shortcuts sourcegraph#500

Fixes #262

Testing plan:

Visit any file, click the command palette button, it should actually open and not disappear.

I have tested on:

• Chrome
• Firefox
• Safari
• Phabricator Bundle

[sqs]

Thanks for fixing this. I completely forgot to follow through with this and get it merged into the browser extension, too.

[felixfbecker]

I would really like to avoid forks as much as possible. Can you open the PR to @shopify/react-shortcuts first? The folks there could review the change better than we can, and it looks very small.

Requiring a ShortcutProvider to be used up in the tree without having any type safety that enforces it also doesn't seem that great. @sqs Maybe we should not use @shopify/react-shortcuts after all?

[sqs]

@sqs Maybe we should not use @shopify/react-shortcuts after all?

I didn’t see any other shortcut libraries that fixed your particular concern (typesafety of context), so I don’t think you’d fix that issue by switching. My assessment is that there weren’t any better libraries than this one (see the commit that introduced it for criteria). But if you find a better one or have different requirements, go for it.

[felixfbecker]

Do we need a shortcut library?

[sqs]

Do we need a shortcut library?

In hindsight, knowing that it needed this change and that in general we have a lot more requirements for shortcuts than the average app, I would actually probably have built our own and not used a library.

[slimsag]

• I've created an upstream PR: react-shortcuts: export Provider for enabling external ShortcutProvider implementors Shopify/quilt#375
• I think it would be silly of us to throw out this work on shortcuts given we've already done it and I don't see a much greater approach. We can improve on this work in the future instead (maybe by making a real fork of react-shortcuts which removes the ShortcutProvider entirely?)
• I will warn everyone in #dev-announce to not release the browser extension until this issue is fixed.

[ijsnow]

I'm fine with merging this temporarily but I'd like to reiterate Felix's question of needing a library for this. I don't think handling keyboard shortcuts is something that should be done by react components as this library is doing. I don't know what all we need out of shortcuts but I think it can easily be handled without a library. For browser extensions, there's even an API built for this https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/commands.

Fix the tslint error before merging though so CI can pass

[sourcegraph-bot]

🎉 This PR is included in version 1.18.0 🎉

The release is available on:

• Firefox Add-on
• Chrome Web Store
• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀