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

Jump to bottom

fix: Avoid multiple browser history push when navigating or using search bar. #1351

Merged
merged 1 commit into from
Nov 11, 2024
Merged

fix: Avoid multiple browser history push when navigating or using search bar. #1351

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
22 changes: 13 additions & 9 deletions packages/frontend/src/components/Header/SearchBar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import { MagnifyingGlassIcon, MultiplyIcon } from '@navikt/aksel-icons';
import cx from 'classnames';
import { useState } from 'react';
import { useEffect, useState } from 'react';
import { useTranslation } from 'react-i18next';
import { useNavigate, useSearchParams } from 'react-router-dom';
import { useSearchParams } from 'react-router-dom';
import { useWindowSize } from '../../../utils/useWindowSize.tsx';
import { Backdrop } from '../Backdrop';
import { useSearchString } from '../Header';
Expand All @@ -15,7 +15,6 @@ export const SearchBar: React.FC = () => {
const [searchParams, setSearchParams] = useSearchParams();
const { searchString, setSearchString } = useSearchString();
const [searchValue, setSearchValue] = useState<string>(searchString);
const navigate = useNavigate();
const { isTabletOrSmaller } = useWindowSize();

const handleClose = () => {
Expand All @@ -26,7 +25,7 @@ export const SearchBar: React.FC = () => {
const newSearchParams = new URLSearchParams(searchParams);
if (newSearchParams.has('data')) {
newSearchParams.delete('data');
setSearchParams(newSearchParams);
setSearchParams(newSearchParams, { replace: true });
}
setSearchValue('');
setSearchString('');
Expand All @@ -42,13 +41,18 @@ export const SearchBar: React.FC = () => {
setSearchString(value);
setShowDropdownMenu(false);
newSearchParams.set('search', value);
setSearchParams(newSearchParams);
navigate({
pathname: '/',
search: `?${newSearchParams.toString()}`,
});
setSearchParams(newSearchParams, { replace: true });
};

useEffect(() => {
const searchBarParam = new URLSearchParams(searchParams);
if (searchBarParam.get('search')) {
return;
}
setSearchValue('');
searchBarParam.delete('search');
}, [location.search]);
Comment on lines +47 to +54
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

⚠️ Potential issue

Fix useEffect dependencies and add missing import

The review comment was correct about the implementation issues. Looking at the file content:

  1. location is not imported but used in dependency array
  2. The component only imports useSearchParams from 'react-router-dom'
  3. The effect should use searchParams in the dependency array since that's what it's actually using

Apply this fix:

- }, [location.search]);
+ }, [searchParams]);

The current implementation is problematic because:

  • It references an undefined location variable
  • The actual data being used (searchParams) is not in the dependency array
  • The effect's logic is already correctly working with searchParams, making location.search unnecessary
🔗 Analysis chain

Fix useEffect implementation issues

There are several issues with the current useEffect implementation:

  1. location.search is used in dependency array but not imported/defined
  2. searchParams should be in the dependency array since it's used in the effect
  3. The effect might cause unnecessary state updates when URL changes

Apply this fix:

  useEffect(() => {
    const searchBarParam = new URLSearchParams(searchParams);
    if (searchBarParam.get('search')) {
      return;
    }
    setSearchValue('');
    searchBarParam.delete('search');
-  }, [location.search]);
+  }, [searchParams]);

Let's verify if location is properly imported elsewhere:

The previous command failed due to typescript extension issue. Let's try a broader search and also look at the actual file content to understand how location is being used and if there are any imports we might have missed.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if location is imported from react-router-dom
rg "import.*location.*from 'react-router-dom'" --type typescript

Length of output: 107

Script:

#!/bin/bash
# Check if location is imported from react-router-dom with broader search
rg "import.*location.*from ['\"]react-router-dom['\"]"

# Also check for useLocation hook usage
rg "useLocation"

# Check the actual file content to understand the context better
cat "packages/frontend/src/components/Header/SearchBar.tsx"

Length of output: 4917


return (
<div className={styles.searchbarContainer}>
<div className={cx(styles.inputContainer, { [styles.searchbarOpen]: showDropdownMenu })}>
Expand Down
2 changes: 1 addition & 1 deletion packages/frontend/src/pages/Inbox/Inbox.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ export const Inbox = ({ viewType }: InboxProps) => {
useEffect(() => {
const newSearchParams = new URLSearchParams(searchParams);
newSearchParams.set('sortBy', selectedSortOrder);
setSearchParams(newSearchParams);
setSearchParams(newSearchParams, { replace: true });
Copy link

@coderabbitai coderabbitai bot Nov 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

💡 Codebase verification

Most setSearchParams calls already use { replace: true }, but one in SearchBar needs attention

Found one instance in packages/frontend/src/components/Header/SearchBar.tsx that doesn't use the replace option:

  • Line: setSearchParams(newSearchParams); when clearing search data

All other instances already properly use { replace: true }:

  • In useParties.ts: All three calls use { replace: true }
  • In SearchBar.tsx: Search value update uses { replace: true }
  • In Inbox.tsx: Sort order update uses { replace: true }
🔗 Analysis chain

LGTM! Prevents duplicate history entries for sort order changes.

The addition of { replace: true } to setSearchParams correctly prevents new history entries when changing sort order, aligning with the PR's objective.

Let's verify other URL parameter updates in the codebase:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for other setSearchParams calls that might need similar treatment

# Search for setSearchParams calls without replace option
rg "setSearchParams\([^)]*\)" --type ts --type tsx -A 2 -B 2

Length of output: 90

Script:

#!/bin/bash
# Let's try a different approach using ripgrep without file type filters
# and then filter results in post-processing

# Search for setSearchParams calls
rg "setSearchParams\(" -A 2 -B 2

Length of output: 2372

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TY rabbit!

Copy link

Choose a reason for hiding this comment

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

@mbacherycz You're welcome!

／(=^ x ^=)＼

}, [selectedSortOrder]);

const shouldShowSearchResults = !isFetchingSearchResults && showingSearchResults;
Expand Down
27 changes: 14 additions & 13 deletions packages/frontend/tests/stories/loginPartyContext.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,21 +110,22 @@ test.describe('LoginPartyContext', () => {
await expect(page.getByRole('link', { name: 'Melding om bortkjøring av snø' })).not.toBeVisible();
});

test('Go-back button updates state and shows correct data and color theme', async ({ page }: { page: Page }) => {
await expect(page.getByRole('button', { name: 'Test Testesen' })).toBeVisible();
await expect(page.getByRole('link', { name: 'Skatten din for 2022' })).toBeVisible();
await expect(page.getByRole('link', { name: 'Innkalling til sesjon' })).not.toBeVisible();
test('Go-back button deletes search bar value', async ({ page }: { page: Page }) => {
await page.getByPlaceholder('Søk i innboks').click();
await expect(page.getByPlaceholder('Søk i innboks')).toBeVisible();
await page.getByPlaceholder('Søk i innboks').fill('skatten din');
await page.getByPlaceholder('Søk i innboks').press('Enter');

await page.getByRole('button', { name: 'Test Testesen' }).click();
await page.getByText('Alle virksomheter').click();
await expect(page.getByRole('link', { name: 'Skatten din for 2022' })).not.toBeVisible();
await expect(page.getByRole('link', { name: 'Innkalling til sesjon' })).toBeVisible();
await expect(page.getByTestId('pageLayout-background')).toHaveClass(/.*isCompany.*/);
let searchParams = new URL(page.url()).searchParams;
expect(searchParams.has('search')).toBe(true);
expect(searchParams.get('search')).toBe('skatten din');
await expect(page.getByRole('link', { name: 'Skatten din for 2022' })).toBeVisible();
await expect(page.getByRole('link', { name: 'Melding om bortkjøring av snø' })).not.toBeVisible();

await page.goBack();
await expect(page.getByRole('button', { name: 'Test Testesen' })).toBeVisible();
await expect(page.getByRole('link', { name: 'Skatten din for 2022' })).toBeVisible();
await expect(page.getByRole('link', { name: 'Innkalling til sesjon' })).not.toBeVisible();
await expect(page.getByTestId('pageLayout-background')).not.toHaveClass(/.*isCompany.*/);
searchParams = new URL(page.url()).searchParams;
expect(searchParams.has('search')).toBe(false);
await expect(page.getByPlaceholder('Søk i innboks')).toBeEmpty();
await expect(page.getByRole('link', { name: 'Melding om bortkjøring av snø' })).toBeVisible();
});
});
Loading