fix: Avoid multiple browser history push when navigating or using search bar. #1351
Conversation
|
Warning Rate limit exceeded@mbacherycz has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 39 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
mbacherycz
force-pushed
the
fix/avoid_multiple_browser_history_push
branch
from
f65bdf1
to
eb7c0f8
Compare
mbacherycz
force-pushed
the
fix/avoid_multiple_browser_history_push
branch
from
eb7c0f8
to
03961df
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
packages/frontend/src/components/Header/SearchBar.tsx (1)
Line range hint
15-21: Consider centralizing search state managementThe component currently manages search state in multiple places:
- URL parameters via
useSearchParams- Global state via
useSearchString- Local state via
useStateThis could lead to synchronization issues and makes the code harder to maintain.
Consider using a single source of truth for search state, preferably URL parameters, and derive other states from it. This would simplify the logic and make the component more maintainable.
packages/frontend/tests/stories/loginPartyContext.spec.ts (1)
119-129: Consider expanding test coverage for browser navigation.The current test effectively verifies the basic back navigation flow. However, to ensure robust handling of browser history:
- Consider adding tests for:
- Forward navigation after going back
- Multiple back/forward navigations
- History stack depth verification
- Verify that multiple searches don't create duplicate history entries
Example test cases to consider:
// Test forward navigation await page.goBack(); await page.goForward(); searchParams = new URL(page.url()).searchParams; expect(searchParams.has('search')).toBe(true); expect(searchParams.get('search')).toBe('skatten din'); // Test multiple searches await page.getByPlaceholder('Søk i innboks').fill('new search'); await page.getByPlaceholder('Søk i innboks').press('Enter'); await page.goBack(); searchParams = new URL(page.url()).searchParams; expect(searchParams.get('search')).toBe('skatten din');packages/frontend/src/pages/Inbox/Inbox.tsx (1)
138-141: Consider consistent URL parameter handling across all updates.While this change correctly handles sort order updates, there are other places in the component where URL parameters are modified:
- Filter changes (via FilterBar)
- Search string updates
- Initial parameter loading
Consider applying the same
{ replace: true }pattern to these updates to maintain consistent history behavior.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
packages/frontend/src/components/Header/SearchBar.tsx(2 hunks)packages/frontend/src/pages/Inbox/Inbox.tsx(1 hunks)packages/frontend/tests/stories/loginPartyContext.spec.ts(1 hunks)
🔇 Additional comments (3)
packages/frontend/src/components/Header/SearchBar.tsx (2)
3-3: LGTM! Import changes align with PR objective
The removal of useNavigate and addition of useEffect support the goal of improving browser history management.
Also applies to: 5-5
44-44: LGTM! Correct implementation to prevent history stack growth
The addition of { replace: true } is the key fix that prevents multiple history entries by replacing the current history state instead of pushing a new one.
packages/frontend/tests/stories/loginPartyContext.spec.ts (1)
113-117: LGTM! Clear test setup and naming.
The test setup is well-structured and the name accurately reflects its purpose of verifying search bar behavior with browser navigation.
| useEffect(() => { | ||
| const searchBarParam = new URLSearchParams(searchParams); | ||
| if (searchBarParam.get('search')) { | ||
| return; | ||
| } | ||
| setSearchValue(''); | ||
| searchBarParam.delete('search'); | ||
| }, [location.search]); |
There was a problem hiding this comment.
💡 Codebase verification
Fix useEffect dependencies and add missing import
The review comment was correct about the implementation issues. Looking at the file content:
locationis not imported but used in dependency array- The component only imports
useSearchParamsfrom 'react-router-dom' - The effect should use
searchParamsin 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
locationvariable - The actual data being used (
searchParams) is not in the dependency array - The effect's logic is already correctly working with
searchParams, makinglocation.searchunnecessary
🔗 Analysis chain
Fix useEffect implementation issues
There are several issues with the current useEffect implementation:
location.searchis used in dependency array but not imported/definedsearchParamsshould be in the dependency array since it's used in the effect- 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
| @@ -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 }); | |||
There was a problem hiding this comment.
💡 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
There was a problem hiding this comment.
@mbacherycz You're welcome!
／(=^ x ^=)＼
mbacherycz
force-pushed
the
fix/avoid_multiple_browser_history_push
branch
2 times, most recently
from
53c2338
to
b7731f1
Compare
mbacherycz
force-pushed
the
fix/avoid_multiple_browser_history_push
branch
from
b7731f1
to
2ac5628
Compare
Hva er endret?
Dokumentasjon / Storybook / testdekning
storiesiStorybook, eller så er ikke dette relevant.Skjermbilder eller GIFs (valgfritt)
Summary by CodeRabbit
New Features
SearchBarcomponent, improving how search parameters are managed in the URL.Inboxcomponent for better browser history management.Bug Fixes