[command-palette] Guard against failure to highlight a match

[savetheclocktower]

Fixes #911.

Identify the Bug

Not many people know this, but when a package author defines a command, they can specify a description for the command to elaborate on the command’s function. Hardly any packages use this feature — hell, built-in packages don’t use this feature! — but the atom-typescript authors knew it existed, and took advantage of it.

If a command has a description, you can type text that’s only in the description and it’ll match. Hence when the command palette fuzzy-matches against user input, it tries to highlight both the command name and the command description, if it exists.

This is fine. But since we’re now matching against either of two strings, we have to be prepared for a match to fail in either case. #911 illustrates a case in which the user’s input fails to match against the description, then causes an exception because we didn’t imagine that matching could fail.

Description of the Change

Guard against a match failure by falling back to an empty array instead of undefined.

Alternate Designs

This regression was introduced when we moved to atom.ui.fuzzyMatcher, and it’s subtle enough that nobody noticed it until now. The root cause is probably that the match function it replaced from fuzzaldrin returned an empty array when there were no matches, whereas atom.ui.fuzzyMatcher.match seems to return undefined. (In this case, we’d need it to return not just an empty array, but an empty array with a matchIndexes property which is also an empty array.)

That change is probably worth making, but for now we can just guard against a possible undefined value.

Possible Drawbacks

None! But I’m going to hijack this section to talk about a different problem that I discovered when I went to add a test for this fix.

command-palette uses a custom test runner, and I’m pretty sure its tests don’t get run as part of CI. If they did, we would’ve caught this bug in the first place: four tests were already failing, including one that tested this exact scenario.

In one sense this is a devops problem to fix. But this could also be fixed by converting the command-palette tests to use the standard test runner. We can argue about that later!

Verification Process

You can go through the steps enumerated in #911 and verify the fix that way.

In order to verify that fewer tests fail than before, you need to be able to run the tests. I was able to do so by

• opening a terminal to the packages/command-palette folder,
• running npm install to install the custom test runner, then
• running pulsar --test test/command-palette-view.test.js.

Before, four tests were failing; after, only one should fail. That failure is unrelated to this change and is based on an assumption that the fuzzy matcher would highlight the spaces in between words; I didn’t feel like taking the time to fix it.

Release Notes

• Prevented an exception raised in the command palette in certain unusual filtering scenarios.

[mauricioszabo]

A single nitpick, left as a suggestion so you can apply directly, but still an easy approve!