Search for apps by title #762

amcaplan · 2022-11-10T19:08:12Z

WHY are these changes introduced?

Fixes #317

Currently Partners can't select an app that's too far down in their org's list of apps. They need to work around using the --api-key flag. This PR allows Partners to search by app title if their app count exceeds the number we fetch.

Videobin

WHAT is this pull request doing?

I think this is best explained by seeing the code comments, which describe the algorithm at length.

TL;DR if they have >100 apps in the organization, typing to search triggers a call to the Partners API searching for the apps by title.

How to test your changes?

Acting as a 1P developer, connect to the Shopify partners organization and search for an app far down the list.

Measuring impact

How do we know this change was effective? Please choose one:

n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix
Existing analytics will cater for this addition
PR includes analytics changes to measure impact
We stop getting issues and Slack messages complaining (quite reasonably!) about this issue

Checklist

I've considered possible cross-platform impacts (Mac, Linux, Windows)
I've considered possible documentation changes
I've made sure that any changes to dev or deploy have been reflected in the internal flowchart.

matteodepalo · 2022-11-22T10:18:50Z

packages/app/src/cli/prompts/dev.ts

+  const fetchInterval = setInterval(async () => {
+    let input: string | undefined
+    do {
+      input = allInputs.pop()


Do we need to keep track of all inputs? I get that going back is going to be faster this way, but we're also going to hit the API many times, when most likely only the results returned by the last input will be used.
What if instead we performed a debounced search only when the user has stopped typing for like 500ms. This should cover the most common scenario and only perform one query.

I still like the idea of caching results so that if you search for something, add some characters and go back we don't fetch again, but I feel like doing extra queries just in case might be a bit wasteful here for the most common scenario.

I like this idea, also avoids blocking for 1 second when the user is in the middle of typing. I'll think about how to implement efficiently.

I'd use the debounce function from lodash, since we already have it in the project

matteodepalo · 2022-11-22T10:23:15Z

packages/app/src/cli/prompts/dev.ts

+           * Otherwise, display the result of whatever intermediate step is
+           * available, so the user at least sees something is happening.
+           */
+          return cachedFiltered[latestRequest] || cachedFiltered[input]!


I wonder wether this behavior might be a bit surprising if compared with typical web autocomplete. Could we instead display no results for ${input}, or something similar?

I don't fully understand this comment, and I would love some more context.

Which scenario is surprising to you? What would you expect instead?

Sorry if I misunderstood the code but from what I can tell if I type hello and there are no results for "hello", I would be a bit surprised to find results for hell. What I would expect from the UI is to tell me that there are no results. But maybe showing something is better than nothing, even if it's not 100% relevant. Not sure.

So, imagine that I type "hello" and we are loading results as we go.

If we just received results for hell we would show them until the results from hello return, and then even if the hello results are empty, we'd replace the hell results with a statement that no results were found.

So the pecking order is:

Results from the latest input, even if empty > The most recently returned results, if not empty > "Searching..."

This may matter much less anyway if we're using the debouncing strategy mentioned above.

Got it, that makes sense 👍 And yes this will probably matter less if we go with a debouncing strategy.

Previously, if the result wasn't part of the initial set of returned apps, it wasn't found. This fixes the bug.

We only need the API key since we'll follow up by fetching full app info anyway!

github-actions · 2022-12-14T11:56:59Z

Coverage report

St.^❔	Category	Percentage	Covered / Total
🟡	Statements	67.69% (-0.35% 🔻)	3472/5129
🟡	Branches	62.07% (-0.71% 🔻)	1108/1785
🟡	Functions	65.73% (-0.33% 🔻)	869/1322
🟡	Lines	67.84% (-0.32% 🔻)	3303/4869

Show files with reduced coverage 🔻

St.^❔	File	Statements	Branches	Functions	Lines
🔴	`...` / dev.ts	55.07% (-24.52% 🔻)	18.75% (-31.25% 🔻)	66.67% (-21.57% 🔻)	52.46% (-23.73% 🔻)
🟢	`...` / fetch.ts	87.5% (-1.69% 🔻)	60% (-15% 🔻)	87.5%	89.19% (+0.62% 🔼)
🟢	`...` / payload.ts	100%	90.91% (-0.76% 🔻)	100%	100%
🟡	`...` / string.ts	70% (-1.43% 🔻)	100%	53.33% (-3.81% 🔻)	69.23% (+1.23% 🔼)
🔴	`...` / executor.ts	48.72% (-1.28% 🔻)	39.13% (-3.73% 🔻)	33.33%	45.95% (-2.63% 🔻)

Test suite run success

863 tests passing in 435 suites.

Report generated by 🧪jest coverage report action from 3e318d9

matteodepalo

LGTM 👍

amcaplan · 2022-12-14T18:05:27Z

Sorry coverage report, this kinda thing is just going to make you sad. 😢

amcaplan requested a review from a team November 10, 2022 19:08

This comment has been minimized.

Sign in to view

amcaplan force-pushed the search-dynamically-for-apps branch 2 times, most recently from f4fad65 to 39fb85e Compare November 12, 2022 18:36

matteodepalo reviewed Nov 22, 2022

View reviewed changes

amcaplan force-pushed the search-dynamically-for-apps branch from 39fb85e to 684bd77 Compare December 13, 2022 19:34

amcaplan added 21 commits December 14, 2022 13:48

Search dynamically for apps based on user input

504ed7b

Cache results for better performance

72d8b88

Use an event loop to avoid hitting rate limits

dc87138

Store results from all queries for final resolution

a77fb9b

Previously, if the result wasn't part of the initial set of returned apps, it wasn't found. This fixes the bug.

Fix typo

d597abc

Fix broken tests

70bf79b

Poll more often with inexpensive check

33494fd

Short-circuit remote search when we have all results locally

caf8cba

clarify with refactoring and comments

9ef5466

Don't repeat searches

8f91c54

Rename name to title

4016c7c

Prompt users to type when possible to search

515cdab

Lint fixes

972fd1a

Add changeset

06b6b00

Test fixups

65fda8e

Use debounce instead: less code, better UX!

0e72f79

Use promises instead of sleeping

93c9f8b

Refactor for clarity

8254a39

Remove unnecessary full app info returned

7a1a102

We only need the API key since we'll follow up by fetching full app info anyway!

Fix broken test

a682b27

Lint fixes

342542b

amcaplan force-pushed the search-dynamically-for-apps branch from 99c4d35 to 342542b Compare December 14, 2022 11:48

amcaplan force-pushed the search-dynamically-for-apps branch 2 times, most recently from 0b16e71 to 00b1a26 Compare December 14, 2022 12:32

Match user inputs case-insensitively

3e318d9

amcaplan force-pushed the search-dynamically-for-apps branch from 00b1a26 to 3e318d9 Compare December 14, 2022 12:36

matteodepalo approved these changes Dec 14, 2022

View reviewed changes

amcaplan merged commit 33024b3 into main Dec 14, 2022

amcaplan deleted the search-dynamically-for-apps branch December 14, 2022 18:05

shopify-shipit bot had a problem deploying to production December 20, 2022 10:24 Failure

shopify-shipit bot had a problem deploying to production December 20, 2022 10:36 Failure

shopify-shipit bot had a problem deploying to production December 20, 2022 10:39 Failure

shopify-shipit bot temporarily deployed to production December 22, 2022 11:12 Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Search for apps by title #762

Search for apps by title #762

amcaplan commented Nov 10, 2022

This comment has been minimized.

matteodepalo Nov 22, 2022 •

edited

Loading

amcaplan Nov 22, 2022

isaacroldan Nov 24, 2022

matteodepalo Nov 22, 2022 •

edited

Loading

amcaplan Nov 22, 2022

matteodepalo Nov 24, 2022

amcaplan Nov 24, 2022

matteodepalo Nov 25, 2022

github-actions bot commented Dec 14, 2022 •

edited

Loading

matteodepalo left a comment

amcaplan commented Dec 14, 2022

Search for apps by title #762

Search for apps by title #762

Conversation

amcaplan commented Nov 10, 2022

WHY are these changes introduced?

WHAT is this pull request doing?

How to test your changes?

Measuring impact

Checklist

This comment has been minimized.

matteodepalo Nov 22, 2022 • edited Loading

Choose a reason for hiding this comment

amcaplan Nov 22, 2022

Choose a reason for hiding this comment

isaacroldan Nov 24, 2022

Choose a reason for hiding this comment

matteodepalo Nov 22, 2022 • edited Loading

Choose a reason for hiding this comment

amcaplan Nov 22, 2022

Choose a reason for hiding this comment

matteodepalo Nov 24, 2022

Choose a reason for hiding this comment

amcaplan Nov 24, 2022

Choose a reason for hiding this comment

matteodepalo Nov 25, 2022

Choose a reason for hiding this comment

github-actions bot commented Dec 14, 2022 • edited Loading

Coverage report

Test suite run success

matteodepalo left a comment

Choose a reason for hiding this comment

amcaplan commented Dec 14, 2022

matteodepalo Nov 22, 2022 •

edited

Loading

matteodepalo Nov 22, 2022 •

edited

Loading

github-actions bot commented Dec 14, 2022 •

edited

Loading