Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(search): Repeat city searching now working #1134

Merged
merged 8 commits into from
Mar 6, 2024

Conversation

kuck1
Copy link
Contributor

@kuck1 kuck1 commented Feb 29, 2024

Pull Request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Build-related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

If you search for city X and then city Y and then back to X there might be an issue where the search doesn't go through. This has really low user impact, but it came up in my last PR so I decided to look into it.

Issue Number: N/A

What is the new behavior?

  • The new behavior is you can repeatedly search back and forth between duplicate cities and it will load.

Does this introduce a breaking change?

  • Yes
  • No

Other information

Before:

Screen.Recording.2024-02-29.at.6.17.09.AM.mov

After:

Screen.Recording.2024-02-29.at.6.18.23.AM.mov

Technical approach

After it became clear that this was repeated cities causing the issue I looked for why the trigger is not called to search again in that case. The search box is working great, but there seemed to be some caching happening with the geo lookup. It turns out that tan stack's useQuery function has the onSuccess callback that we use but it is only called when new data is fetched. Existing searches apparently are cached by default for 5 minutes and don't trigger a new query. As a result, repeat cities were not triggering onSuccess which is where we set up the parameters to do the final important search.

This is a confusing way tan stack designed this that was widely misunderstood according to stack overflow, so they are deprecating onSuccess in the next major version. I think there are some guides on how to migrate beyond this by using the caching system but that seems to be the scope of another bigger ticket. For now, if we turn the caching off, then onSuccess will be triggered every time and the search will happen as expected.

Official docs mentioning the deprecation

Stackoverflow post discussing the deprecations and work arounds

@kuck1 kuck1 added priority-low Nice addition, maybe... someday... 📦 app labels Feb 29, 2024
@kuck1 kuck1 requested a review from JoeKarow as a code owner February 29, 2024 14:31
Copy link

alwaysmeticulous bot commented Feb 29, 2024

🤖 No test run has been triggered as your Meticulous project has been deactivated (since you haven't viewed any test results in a while). Click here to reactivate.

Last updated for commit ddabf07. This comment will update as new commits are pushed.

Copy link

vercel bot commented Feb 29, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
inreach-app ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 6, 2024 5:16pm

@github-actions github-actions bot added 📦 ui automerge Enable Kodiak auto-merge kodiak: merge.method = 'squash' Kodiak will squash merge this PR. labels Feb 29, 2024
Copy link
Contributor

github-actions bot commented Feb 29, 2024

📦 Next.js Bundle Analysis for @weareinreach/app

This analysis was generated by the Next.js Bundle Analysis action. 🤖

This PR introduced no changes to the JavaScript bundle! 🙌

@JoeKarow JoeKarow added the bugfix Inconsistencies or issues which will cause a problem for users or implementors. label Mar 6, 2024
Copy link

sonarcloud bot commented Mar 6, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link
Member

@JoeKarow JoeKarow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good find! I made a slight change moving the onSuccess callback in to a useEffect. We will be upgrading to tRPC v11/TanStack Query v5 in the near future, which fully deprecates the onSuccess callback - so we should try and refactor those out when we come across them for now.

@kodiakhq kodiakhq bot merged commit 7d13c83 into dev Mar 6, 2024
23 of 24 checks passed
@kodiakhq kodiakhq bot deleted the changed-search-not-always-searching-again branch March 6, 2024 17:16
@kuck1
Copy link
Contributor Author

kuck1 commented Mar 6, 2024

Good find! I made a slight change moving the onSuccess callback in to a useEffect. We will be upgrading to tRPC v11/TanStack Query v5 in the near future, which fully deprecates the onSuccess callback - so we should try and refactor those out when we come across them for now.

Awesome, sounds great!

})

useEffect(() => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 app automerge Enable Kodiak auto-merge bugfix Inconsistencies or issues which will cause a problem for users or implementors. kodiak: merge.method = 'squash' Kodiak will squash merge this PR. priority-low Nice addition, maybe... someday... 📦 ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants