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.

Sign up for GitHub

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

Bug fix - Map not showing full users after clear with large data set #5474

Closed
wants to merge 8 commits into from
Closed

Bug fix - Map not showing full users after clear with large data set #5474

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a673116
Fix pagination not being reset
nabramow Jan 15, 2025
9cffb4f
Test fix
nabramow Jan 15, 2025
16812d0
Try again
nabramow Jan 15, 2025
68c6ba8
WIP
nabramow Jan 15, 2025
dd4cc2d
WIP
nabramow Jan 15, 2025
f394bf1
Clean up
nabramow Jan 15, 2025
d70e36b
Merge branch 'develop' into na/web/bugfix-map-clear
nabramow Jan 15, 2025
b8fe902
Updated state names to be more clear and fixed minor bug
graceegal Jan 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions app/web/features/communities/events/EventTimeChanger.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(() =>
Copy link

Choose a reason for hiding this comment

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

Why is this waitFor being added does this have anything to do with the bug fix? Or just a testing issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

expect(startTimeErrorText).toHaveTextContent(
t("communities:past_time_error")
)
);

user.click(screen.getByTestId("submit"));
Expand Down
16 changes: 5 additions & 11 deletions app/web/features/search/MapWrapper.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,9 +94,8 @@ interface mapWrapperProps {
SetStateAction<Pick<User.AsObject, "userId" | "lng" | "lat"> | undefined>
>;
map: MutableRefObject<MaplibreMap | undefined>;
setWasSearchPerformed: Dispatch<SetStateAction<boolean>>;
wasSearchPerformed: boolean;
areFiltersCleared: boolean;
setMapPositionFilterActive: Dispatch<SetStateAction<boolean>>;
onClearFiltersClick: () => void;
}

Expand All @@ -109,9 +108,8 @@ export default function MapWrapper({
results,
setSelectedResult,
setIsFiltersOpen,
wasSearchPerformed,
setWasSearchPerformed,
areFiltersCleared,
setMapPositionFilterActive,
onClearFiltersClick,
}: mapWrapperProps) {
const { t } = useTranslation([SEARCH]);
Expand Down Expand Up @@ -240,11 +238,7 @@ export default function MapWrapper({
* Re-renders users list on map (when results array changed)
*/
useEffect(() => {
if (
isMapStyleLoaded &&
isMapSourceLoaded &&
(wasSearchPerformed || areFiltersCleared)
) {
if (isMapStyleLoaded && isMapSourceLoaded && areFiltersCleared) {
if (results) {
const usersToRender = filterData(results);
reRenderUsersOnMap(map.current!, usersToRender, handleMapUserClick);
Expand All @@ -256,8 +250,8 @@ export default function MapWrapper({
map,
isMapStyleLoaded,
isMapSourceLoaded,
wasSearchPerformed,
areFiltersCleared,
// wasSearchPerformed, (FIXME: revisit whether I'll need mapPositionFilterActive here or not)
]);

/**
Expand All @@ -280,7 +274,7 @@ export default function MapWrapper({
],
});
}
setWasSearchPerformed(true);
setMapPositionFilterActive(true);
}
};

Expand Down
24 changes: 12 additions & 12 deletions app/web/features/search/SearchPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,9 +81,8 @@ export default function SearchPage({
const isMobile = useMediaQuery(theme.breakpoints.down("md"));

// State
const [wasSearchPerformed, setWasSearchPerformed] = useState(
locationName !== ""
);

const [mapPositionFilterActive, setMapPositionFilterActive] = useState(false);
const [locationResult, setLocationResult] = useState<GeocodeResult>({
bbox: bbox,
isRegion: false,
Expand All @@ -107,6 +106,7 @@ export default function SearchPage({
>();

const [isFiltersOpen, setIsFiltersOpen] = useState(false);
// Filters = mapPosition, locationResult, queryName, lastActive, hostingStatus, numberOfGuest & completeProfile
const [areFiltersCleared, setAreFiltersCleared] = useState(
locationName === ""
);
Expand Down Expand Up @@ -163,21 +163,22 @@ export default function SearchPage({
completeProfileFilter !== false ||
queryName !== "" ||
locationResult.name !== "" ||
wasSearchPerformed !== false;
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

mapPositionFilterActive === true;
// (locationResult.location.lng !== 0 && locationResult.location.lat !== 0);

if (!wasSearchPerformed && filtersApplied) {
setWasSearchPerformed(true);
}
// if (!wasSearchPerformed) {
// setWasSearchPerformed(true);
// }

setAreFiltersCleared(!filtersApplied);
}, [
lastActiveFilter,
hostingStatusFilter,
numberOfGuestFilter,
completeProfileFilter,
wasSearchPerformed,
queryName,
locationResult.name,
mapPositionFilterActive,
]);

/**
Expand All @@ -196,8 +197,8 @@ export default function SearchPage({
setHostingStatusFilter([]);
setNumberOfGuestFilter(undefined);
setCompleteProfileFilter(false);
setMapPositionFilterActive(false);
setAreFiltersCleared(true);
setWasSearchPerformed(false);
};

const errorMessage = error?.message;
Expand Down Expand Up @@ -226,7 +227,7 @@ export default function SearchPage({
{/* Mobile */}
{isMobile && (
<Collapse
in={wasSearchPerformed || !!selectedResult}
in={!!selectedResult}
timeout={theme.transitions.duration.standard}
className={classes.mobileCollapse}
>
Expand Down Expand Up @@ -271,9 +272,8 @@ export default function SearchPage({
setLocationResult={setLocationResult}
setSelectedResult={setSelectedResult}
isLoading={isLoading || isFetching}
setWasSearchPerformed={setWasSearchPerformed}
wasSearchPerformed={wasSearchPerformed}
areFiltersCleared={areFiltersCleared}
setMapPositionFilterActive={setMapPositionFilterActive}
onClearFiltersClick={handleClearFilters}
/>
</div>
Expand Down