Skip to content

Clean up and refactor global shortcuts

Mark Florian requested to merge 392845-refactor-global-shortcuts into master

What does this MR do and why?

This is the first MR towards #392845 (closed).


chore: Remove dead event listener

The .js-more-help-button element was removed nearly three years ago in !34660 (merged), so this event listener will never get called.

chore: Remove dead code due to unused arguments

  • The skipResetBindings constructor parameter was removed in 4ee29e89, so all related code can be removed.
  • The ShortcutsEpic.openSidebarDropdown static method's only parameter was removed in !75302 (merged), so there's no point passing anything to it.
  • The ShortcutsTestCase.openSidebarDropdown static method never expected any parameters since creation in !109527 (merged), so there's no point passing anything to it.

test: Improve Shortcuts specs

  • Only instantiate Shortcuts once, since constructing it has side effects, and there's no teardown method.
  • Spy on Mousetrap methods instead of mocking it entirely. It turns out that the stopCallback tests weren't passing for the right reason as a result. (There's a difference between the global Mousetrap instance and the one set up for Shortcuts#initMarkdownEditorShortcuts`.)
  • Rename shortcuts variable to more explicit shortcutElements.

refactor: Add and use Shortcut#bindCommand(s) methods

These encapsulate the logic to call keysFor and Mousetrap.bind. This will allow for simpler calls in Shortcuts and its subclasses.

They are instance methods rather than static methods to encourage access via the (in theory) singleton Shortcuts instance.

The call refactoring was partially implemented using this command:

comby "Mousetrap.bind(keysFor(:[[command]]), :[callback]);" \
    "[:[command], :[callback]]," \
    {ee/,}app/assets/javascripts/behaviors/shortcuts/*.js

See https://comby.dev/ for details about comby.

The actual calls to this.bindCommand(s) were then done by hand, along with some minor code rearrangement in the Shortcuts constructor.

refactor: Extract Mousetrap util file

This new ~/lib/utils/mousetrap file is similar to ~/lib/utils/axios_utils, in that it augments the raw package.

In particular, it makes it slightly easier to update Mousetrap's stopCallback method.

They Shortcut#addStopCallback is an instance method rather than static methods to encourage access via the (in theory) singleton Shortcuts instance.

A problem this does not solve is the race between our overwriting of Mousetrap#stopCallback and the pause plugin's overwriting of it: https://github.com/ccampbell/mousetrap/blob/1.6.5/plugins/pause/mousetrap-pause.js#L10-L18.

If the pause plugin is loaded first, then our subsequent overwrites will no longer be subject to the paused status of Mousetrap.

The bulk import path rename was done using this command:

comby "import :[[name]] from 'mousetrap';" \
    "import :[name] from '~/lib/utils/mousetrap';" \
    {ee/,}{spec/frontend,app/assets/javascripts}/**/*.{js,vue} -i

See https://comby.dev/ for details about comby.

Finally, a redundant Mousetrap Jest mock has been removed, as the package has provided a CommonJS module since v1.5.0: https://github.com/ccampbell/mousetrap/releases/tag/1.5.0

Use named export

We do technically lint for default exports, but currently opt out in GitLab due of the sheer number of violations. Anyway, we might as well use a named export since we're wrapping Mousetrap.

Document Mousetrap pause plugin compatibility

Remove unnecessary method

The indirection serves no purpose, so just use the imported function directly.

Fix incorrect JSDoc type

Improve JSDoc types

  • Document callback signature.
  • There is no return value, so use void instead of undefined.

Improve description of addStopCallback

Screenshots or screen recordings

N/A, no changes.

How to set up and validate locally

  1. Press ? to open shortcuts help modal.
  2. Choose some shortcuts to test.
  3. Visit the relevant pages for them.
  4. Press the shortcuts.
  5. Ensure they do as advertised.

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Related to #392845 (closed)

Edited by Mark Florian

Merge request reports

Loading