fix: Add search parameters to inbox message link #1366
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces changes to the Changes
Possibly related PRs
Suggested reviewers
Warning Rate limit exceeded@mbacherycz has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 3 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. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/frontend/src/components/InboxItem/InboxItem.tsx (1)
103-108: Add test coverage for URL parameter handlingConsider adding test cases to verify:
- URL parameter preservation across navigation
- Handling of undefined
linkTo- Handling of existing search parameters in
linkToWould you like me to help generate test cases for these scenarios?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
packages/frontend/src/components/InboxItem/InboxItem.tsx(5 hunks)
🔇 Additional comments (4)
packages/frontend/src/components/InboxItem/InboxItem.tsx (4)
4-4: LGTM: Import statement correctly updated
The addition of useLocation hook from react-router-dom is appropriate for accessing URL search parameters.
103-103: LGTM: Proper hook usage
The useLocation hook is correctly implemented at the component level, following React hooks best practices.
133-133: LGTM: Consistent link handling across views
The linkToWithParams is consistently applied in both minimalistic and standard views, maintaining the existing selection mode logic.
Also applies to: 179-179
168-168: Verify hoverable states in all scenarios
The hoverable class now depends on linkToWithParams. Please verify that the UI states are correct in these scenarios:
- When
linkTois undefined - When in selection mode
- When URL has no search parameters
✅ Verification successful
Hoverable state implementation is correct and well-handled
The code correctly handles all the scenarios:
- When
linkTois undefined,linkToWithParamswill be undefined, and hoverable class won't be applied - In selection mode, the link is disabled (
!inSelectionMode ? linkToWithParams : undefined) and hoverable state won't apply - When URL has search parameters, they are properly appended (
${linkTo}${location.search})
The implementation also maintains clear separation between hoverable and pointer states:
[styles.hoverable]for link navigation[styles.pointer]for selection mode interactions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for tests covering hoverable states
rg -l "hoverable.*inboxItem" --type=test
Length of output: 71
Script:
#!/bin/bash
# Search for test files related to InboxItem
fd -e test.tsx -e test.ts -e spec.tsx -e spec.ts InboxItem
# Search for hoverable class usage in InboxItem tests and component
rg -A 5 "hoverable" packages/frontend/src/components/InboxItem/
Length of output: 1455
Script:
#!/bin/bash
# Search for linkTo and linkToWithParams usage in InboxItem
rg -A 5 "linkTo(WithParams)?" packages/frontend/src/components/InboxItem/InboxItem.tsx
# Search for selection mode related code
rg "inSelectionMode|isChecked" packages/frontend/src/components/InboxItem/InboxItem.tsx
Length of output: 2478
| const linkToWithParams = `${linkTo}${location.search}`; | ||
|
|
There was a problem hiding this comment.
Add URL parameter handling safety checks
The current implementation has potential issues:
- No validation for undefined
linkTo - No handling of existing search parameters in
linkTo - Could result in malformed URLs if
linkToalready contains search parameters
Consider using URL utilities for safer parameter handling:
- const linkToWithParams = `${linkTo}${location.search}`;
+ const linkToWithParams = linkTo
+ ? new URL(linkTo, window.location.href).search
+ ? linkTo // If linkTo already has search params, leave it unchanged
+ : `${linkTo}${location.search}`
+ : undefined;Committable suggestion skipped: line range outside the PR's diff.
|
|
||
| const hideSummaryAndMeta = !summary || viewType === 'archive' || viewType === 'bin'; | ||
|
|
||
| const linkToWithParams = `${linkTo}${location.search}`; |
There was a problem hiding this comment.
Could I do suggest a little improvement here?
InboxItem should probably be so dumb as possible, and rather, let this linkTo + location.searche be lifted to Inbox. What do you think of the following:
- remove the prop
linkTofromInboxItemInput(both fra the interface and from the item-mapping inuseDialogs.tsx. - In
Inbox.tsxwhere the items are mapped intoInboxItem, build thelinkToprop with following pattern/inbox/${item.id}/${location.search} - Do the same for
InboxItemused inSearchDropdown
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/frontend/src/components/Header/SearchDropdown.tsx (1)
70-70: Consider centralizing URL construction logicThe codebase has multiple locations where URLs are constructed with search parameters (both in
linkToandqueryParamsconstruction). This could lead to inconsistencies in URL handling.Consider creating a centralized utility for URL construction to ensure consistent handling of search parameters across the application.
Example approach:
// utils/urlBuilder.ts export const buildUrl = { inbox: (id: string, searchParams: string) => `/inbox/${id}${searchParams}`, search: (params: Record<string, string>) => { const queryParams = new URLSearchParams(params); return `?${queryParams.toString()}`; } };Also applies to: 98-102
packages/frontend/src/pages/Inbox/Inbox.tsx (1)
378-378: Improve URL construction for inbox item linksThe addition of search parameters to the inbox item link implements the PR objective correctly. However, the URL construction could be improved to handle edge cases.
Consider using URL utilities to handle the search parameters more robustly:
- linkTo={`/inbox/${item.id}/${location.search}`} + linkTo={`/inbox/${item.id}${location.search ? location.search : ''}`}This change:
- Prevents trailing slash when there are no search parameters
- Maintains the existing search parameters as intended
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
packages/frontend/src/api/useDialogs.tsx(0 hunks)packages/frontend/src/components/Header/SearchDropdown.tsx(3 hunks)packages/frontend/src/components/InboxItem/InboxItem.tsx(1 hunks)packages/frontend/src/pages/Inbox/Inbox.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- packages/frontend/src/api/useDialogs.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/frontend/src/components/InboxItem/InboxItem.tsx
🔇 Additional comments (3)
packages/frontend/src/components/Header/SearchDropdown.tsx (3)
5-5: LGTM: Import addition is appropriate
The addition of useLocation hook is necessary for accessing URL search parameters and maintaining them during navigation.
34-34: LGTM: Hook usage follows best practices
The useLocation hook is correctly initialized at the component level and follows React hooks rules.
70-70:
Fix URL construction to prevent invalid format
The current URL construction '/inbox/${item.id}/${location.search}' will result in an invalid URL format because:
location.searchalready includes the?prefix- Adding a
/before the search string creates an invalid URL structure (e.g.,/inbox/123/?search=testbecomes/inbox/123//search=test)
Apply this fix to ensure proper URL construction:
- linkTo={`/inbox/${item.id}/${location.search}`}
+ linkTo={`/inbox/${item.id}${location.search}`}Let's verify the search parameter handling in other components:
mbacherycz
force-pushed
the
fix/context_colour_switches_to_person_when_opening_company_message
branch
from
ecdd08c to
791ba26
Compare
mbacherycz
force-pushed
the
fix/context_colour_switches_to_person_when_opening_company_message
branch
from
791ba26 to
698572b
Compare
mbacherycz
deleted the
fix/context_colour_switches_to_person_when_opening_company_message
branch
Hva er endret?
Dokumentasjon / Storybook / testdekning
storiesiStorybook, eller så er ikke dette relevant.Skjermbilder eller GIFs (valgfritt)
Summary by CodeRabbit
New Features
Bug Fixes