Bug fix - Map not showing full users after clear with large data set #5474
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
nabramow
changed the title
|
I just tried testing this on mobile and it's proving hard to test because the number of test users on vercel/next appears to be the same or similar to the max count of users (100) that we display due to pagination (that was how the bug made it through to production in the first place). I encountered a new issue during testing: the "clear filters" button remains grayed out despite a filter being applied. How to replicate: Search for Berlin. The search zooms in and takes you to berlin. Then move the map around a bit and click "search here". Despite the location filter still being applied, the "clear filters" button is still grayed out. I'm on my way to work right now, I can post a video of my screen after work if needed. |
| @@ -148,8 +148,10 @@ it("should show proper error if startDate is today but startTime is in the past" | |||
|
|
|||
| const startTimeErrorText = await screen.findByTestId("startTime-helper-text"); | |||
|
|
|||
| expect(startTimeErrorText).toHaveTextContent( | |||
| t("communities:past_time_error") | |||
| await waitFor(() => | |||
There was a problem hiding this comment.
Why is this waitFor being added does this have anything to do with the bug fix? Or just a testing issue?
There was a problem hiding this comment.
Ah this was a comment Aapeli made during the meeting that a test was failing on develop after this PR merged, and he sent me this test as the culprit. There's some tests after the React v18 upgrade randomly failing still as they run slower on deployment then locally so we didn't catch them.
| @@ -163,9 +163,9 @@ export default function SearchPage({ | |||
| completeProfileFilter !== false || | |||
| queryName !== "" || | |||
| locationResult.name !== "" || | |||
| wasSearchPerformed !== false; | |||
| (locationResult.location.lng !== 0 && locationResult.location.lat !== 0); | |||
There was a problem hiding this comment.
Not that it is really negatively affecting anything by having it but just to shed some light on why I decided to not use locationResult.location.lng/lat previously...
locationResult.name provides more reliable state tracking for searches because:
-
On initial search from the dashboard:
locationResult.nameupdates correctly (via URL params)locationResult.locationcoordinates remain at 0
-
On subsequent searches from within the map:
- Both
locationResult.nameand coordinates update as expected
- Both
Since locationResult.name consistently indicates whether a specific place was searched for across both initial and subsequent searches, it's a more reliable indicator than the coordinate values. Adding a dependency on locationResult.location seemed redundant/ potentially misleading due to its inconsistent behavior on initial load.
There was a problem hiding this comment.
@graceegal with the locationResult.location.lng/lat vs locationResult.name, from my memory you're not always guaranteed to have both. If you scroll around the map then hit "searchHere", for example, I think you only have bbox? Please check me though I might be remembering wrong! I remember from a past ticket that certain types of searches don't give you everything.
| @@ -163,9 +163,9 @@ export default function SearchPage({ | |||
| completeProfileFilter !== false || | |||
| queryName !== "" || | |||
| locationResult.name !== "" || | |||
| wasSearchPerformed !== false; | |||
There was a problem hiding this comment.
I just tried testing this on mobile and it's proving hard to test because the number of test users on vercel/next appears to be the same or similar to the max count of users (100) that we display due to pagination (that was how the bug made it through to production in the first place).
I encountered a new issue during testing: the "clear filters" button remains grayed out despite a filter being applied. How to replicate:
Search for Berlin. The search zooms in and takes you to berlin. Then move the map around a bit and click "search here". Despite the location filter still being applied, the "clear filters" button is still grayed out.
I'm on my way to work right now, I can post a video of my screen after work if needed.
Just replicated @jesseallhands new issue and checked it out - I think it is happening because this line was removed. If you add a different filter, besides location, and follow the same steps he outlined, the button works as intended and doesn't grey out.
Quick note on the "Search here" button - it resets locationResult to avoid conflicts between:
- The original location search (e.g., "Berlin")
- The new position-based search from
search here
This makes sense since having both would be contradictory (if you search "Berlin", move to NYC and hit "Search here", you wouldn't want both active).
So without the removed line, the clear filters button's enabled state doesn't account for search here searches, making it appear disabled even though the map position is filtering the results.
There was a problem hiding this comment.
Maybe I'm getting confused, isn't filtersApplied what we use to decide wasSearchPerformed? So they are representing the same thing? Gosh I hate this map lol.
|
Closing, the scope of this exploded quite a bit. We will leave it for now as I will try and redo the map soon. |
We introduced a bug in #5413 . It was due to there being a smaller data set on stage, so hard to notice. To replicate on prod:
This PR fixes that. You should repeat those steps above but be able to go back to your initial view.
Web frontend checklist
yarn formatyarn lint --fix