Keep QuickOpenModal resultCount in bounds

[arosenfeld2003]

Fixes #8119

Here's the Pull Request Doc
https://firefox-dev.tools/debugger/docs/pull-requests.html

Summary of Changes

Added a conditional to keep resultCount (number returned from getResultCount) at 100 or less.
This is contained in the traverseResults method, within QuickOpenModal.js.

Test Plan

Tested behavior based upon expected behavior described by @fvsch.

Example test plan:

• There are 100 items displayed in Go To File input.
• Clicking "Up" arrow key from index 0 cycles back to the last item (item 99).
• Clicking the "Down" arrow from index 99 cycles up to the first list item (index 0).
• Passed all unit tests locally.

Screenshots/Videos (OPTIONAL)

This is my first code PR, so greatly appreciate any and all feedback.
Sorry for the extraneous commits (issues with git).
Thanks!

[fvsch]

So I think the fix to use depends on if we want to use a result count of more than the limit (100) to do some computations and/or show in the interface. For instance, do we want to show "172 results" somewhere in the UI, even though we only show and give access to the first 100 results?

Option 1: slice results in state

If we're okay with clipping to 100 for all intents and purposes, the simpler fix would be to clip the results before we add them to the component's state, here:

debugger/src/components/QuickOpenModal.js

Line 124 in 94b063e

return this.setState({ results });

Basically changing this.setState({ results }); to:

this.setState({ results: results.slice(0, maxResults) });

And removing the corresponding code in

debugger/src/components/QuickOpenModal.js

Line 377 in 94b063e

const newResults = results && results.slice(0, 100);

(then we don't need newResults, we can just pass this.state.results directly since it's already sliced).

Option 2: keep full results in state

Now if we want to keep the full results array in the component's state but make calculations against its length or the limit, whichever is smallest, then the technique you used is alright.

In that scenario, we definitely need a single const maxResults = 100; somewhere (near the start of the file, probably), and we need to replace the hardcoded 100 limit in the render() method with that constant too.

Nitpick: you can probably simplify your condition a bit by using Math.min:

const resultCount = Math.min(this.getResultCount(), maxResults);

[arosenfeld2003]

Thank you @jasonLaster and @fvsch. I had the same question about whether the full results were needed elsewhere in the UI. Your suggestions make a lot of sense. I'm happy to go either direction depending on what's needed.

As an additional option, my first instinct was a simple change of maxResults (set in the filter function, line 75) from 1000 to 100.

function filter(values, query) {
  return fuzzyAldrin.filter(values, query, {
    key: "value",
    maxResults: 100
  });
}

This solved the problem, and I thought was much cleaner than my initial solution, but it failed three tests that rely on maxResults: 1000 ...

QuickOpenModal.spec.js - lines 212, 233, 260.

If only 100 results are ever needed, the filter could be set at 100, the tests run with maxResults: 100, and all other behavior/tests work as expected.

[arosenfeld2003]

@fvsch and @jasonLaster great suggestions! I've got three workable solutions based on your advice.

Keep Full Results In State
The simplest change keeps full results in state, updating traverseResults() as follows:

   traverseResults = (e: SyntheticKeyboardEvent<HTMLElement>) => {
     const maxResults = 100;
     const direction = e.key === "ArrowUp" ? -1 : 1;
     const { selectedIndex, results } = this.state;
     const resultCount = Math.min(this.getResultCount(), maxResults);

Slice Results In State
If we slice results in state based on your suggestion above, the searchSources and render() methods are updated. Applying slice() throws an error when results is undefined, which fails a series of tests. I added a conditional to return results if undefined without applying slice, which passed all tests.

searchSources: (line 120)

  const { sources } = this.props;
  const results =
    query == "" ? sources : filter(sources, this.dropGoto(query));
     if (results) {
        return this.setState({ results: results.slice(0, 100) });  
     }
     return this.setState({ results });
   };

render: (line 378)

const items = this.highlightMatching(query, results || []);
const expanded = !!items && items.length > 0;

render: (line 401)

       {results && (
           <ResultList
             key="results"
             items={items}

Limit Results In Filter
Another solution that works is setting maxResults: 100 in the filter method, with same changes to render() as above. This also required setting maxResults: 100 in the three tests based on that value.

filter function: (line 72)

 function filter(values, query) {
   return fuzzyAldrin.filter(values, query, {
     key: "value",
     maxResults: 100
   });
 }

I feel like if we want to clip all results, changing maxResults in the filter makes more sense? But I'm happy to go any direction based on what you think is needed.

Thanks for all your help!

[fvsch]

Given that it's a "quick open" feature, not "what's the exact count of sources that match this query" feature, I believe we should just limit the number of results we deal with. Can you try the "Limit Results In Filter" approach?

[WenInCode]

I think I agree with @fvsch on that we should just limit the results returned from the filter function.
I don't think that will cover all the cases though. For instance, if I open a file with more than 100 symbols and do an empty symbol query @ it will just return all the symbols before running filter.

searchSymbols = (query: string) => {
    const {
      symbols: { functions }
    } = this.props;

    let results = functions;
    results = results.filter(result => result.title !== "anonymous");

    if (query === "@" || query === "#") {
      return this.setState({ results });  <--- this could be more than 100 results
    }

    this.setState({ results: filter(results, query.slice(1)) });  <--- If the query is not empty it will use filter
  };

I'll include a reproduction of that.
STR:

• go to https://lodash.glitch.me/
• pretty print and inspect https://code.jquery.com/jquery-2.2.1.min.js in the devtools
• open the QuickOpenModal and enter @ to get all the symbols in the file.
• tap the up arrow key a few times

Great work so far @arosenfeld2003 - you are asking great questions. I like the filter approach, but I think there are just a few cases that doesn't cover that we should watch out for.

[arosenfeld2003]

@WenInCode, great catch. The edge case above can be handled by slicing results in the searchSymbols method:

searchSymbols = (query: string) => {
    const {
      symbols: { functions }
    } = this.props;

    let results = functions;
    results = results
      .filter(result => result.title !== "anonymous")
      .slice(0, 100);
    if (query === "@" || query === "#") {
      return this.setState({ results });
    }

    this.setState({ results: filter(results, query.slice(1)) });
  };

This seems straightforward, and passes all jest tests locally.

I found four methods in QuickOpenModal where results is passed to setState: searchSources(), searchSymbols(), searchShortcuts(), and showTopSources(). I didn't come up with any other edge cases in these methods where we'd get more than 100 sources in the 'go to file' box, but please let me know if can think of another instance that I missed, or another reason we'd get more sources??

Otherwise, I'm happy to amend my PR with the above changes if you think they are agreeable?

Thank you again for all your help @WenInCode , @fvsch, @jasonLaster.

[WenInCode]

@arosenfeld2003 seems good to me 👌

[fvsch]

Sounds like a good approach.

If we're doing this.setState({ results: results.slice(0, 100) }) in many different functions it might be worthwhile to move that into a function too.

[arosenfeld2003]

Good suggestion @fvsch.

I think it's ok for now, since we don't actually have to call
this.setState({ results: results.slice(0, 100) }).

To catch the edge case above with an empty symbol query, we can slice before setState.
Otherwise if the query is not empty, the filter will limit results to 100.

searchSymbols = (query: string) => {
    const {
      symbols: { functions }
    } = this.props;

    let results = functions;
    results = results
      .filter(result => result.title !== "anonymous")
      .slice(0, 100);

    if (query === "@" || query === "#") {
      return this.setState({ results });  
    }

    this.setState({ results: filter(results, query.slice(1)) });  
  };

In showTopSources, we set results by slicing the sources:

this.setState({ results: sources.slice(0, 100) })

All the help is much appreciated!

[arosenfeld2003]

I'm hoping this is now good to go? Please let me know if anything else is needed. Thanks again.

[fvsch]

Two things:

1. You're hardcoding the value 100 in a few different places, which has a potential for creating bugs if we ever change one place and not the others. As was suggested before, could you put it in a const declared near the start of the file (probably just before this)?

2. Looking at QuickOpenModal, there are 7 places where we call this.setState({ results: someValue }). Making sure that all 7 calls are working with a someValue clipped at 100 items maximum does not seem safe to me. If we missed something or change one of those calls later on, it could lead to this bug happening again in specific conditions. Perhaps we should do all results state update through a single class method, for example:

  setResults = (results) => {
    if (Array.isArray(results) && results.length > maxResults) {
      results = results.slice(0, maxResults);
    }
    this.setState({ results })
  }

[arosenfeld2003]

Thanks so much for the detailed suggestions @fvsch! I now understand what you mean.

I think I've got a good solution that creates/calls a setResults method and a const maxResults = 100 just above the filter. It passes all jest tests locally.

I did get a Flow(InferError) missing type annotation popping up, as seen here:

Unfortunately this is also causing a failed CI. I tried to determine the cause, but after investigation I'm not quite sure what direction to go. I thought that the type for results was being set here:

type State = {
  results: ?Array<QuickOpenResult>,
  selectedIndex: number
};

I would definitely appreciate a nudge in the right direction here... Thank you again for all your time, @fvsch, @WenInCode, @jasonLaster.

[fvsch]

Are you familiar with Flow type annotations? You can read up on them here.

I think here you need to tell Flow that setResults's results parameter should be an array of objects whose type is QuickOpenResult.

[darkwing]

Wow, this looks great @arosenfeld2003 ! Just a quick rebase and it'll be good to merge!

[arosenfeld2003]

Thank you all so much @fvsch @WenInCode @jasonLaster @darkwing ... did a rebase, hopefully it's good to go.

[darkwing]

Hello @arosenfeld2003! I fixed our CI (Travis) so one more quick rebase and this will be super green! :)

[arosenfeld2003]

@darkwing just checking if I need to do anything further here? Thanks again for your help!

[darkwing]

Excellent, thank you @arosenfeld2003 !