Fixes issue #2627 #2682
Fixes issue #2627 #2682
Conversation
WalkthroughThe pull request focuses on improving the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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
|
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (3)
src/screens/OrganizationPeople/AddMember.tsx (3)
Line range hint
92-93: Fix incorrect state variable in toggleCreateNewUserModalThe function is using the wrong state variable (
addUserModalisOpen) instead ofcreateNewUserModalisOpenin the toggle logic, which will cause incorrect modal behavior.Apply this fix:
const toggleCreateNewUserModal = (): void => - setCreateNewUserModalIsOpen(!addUserModalisOpen); + setCreateNewUserModalIsOpen(!createNewUserModalisOpen);
Line range hint
95-108: Enhance error handling in createMember functionWhile the error handling has been improved, it could be more robust to handle all error types consistently.
Consider this enhancement:
const createMember = async (userId: string): Promise<void> => { try { await addMember({ variables: { userid: userId, orgid: currentUrl, }, }); toast.success(tCommon('addedSuccessfully', { item: 'Member' }) as string); memberRefetch({ orgId: currentUrl, }); } catch (error: unknown) { - if (error instanceof Error) { - toast.error(error.message); - console.log(error.message); - } + const errorMessage = error instanceof Error ? error.message : 'An unknown error occurred'; + console.error('Failed to create member:', errorMessage); + toast.error(errorMessage); } };
Modal state management needs correction in toggleCreateNewUserModal
Found an issue where
toggleCreateNewUserModalincorrectly usesaddUserModalisOpeninstead ofcreateNewUserModalisOpen:const toggleCreateNewUserModal = (): void => setCreateNewUserModalIsOpen(!addUserModalisOpen); // Wrong state variableThis should be:
const toggleCreateNewUserModal = (): void => setCreateNewUserModalIsOpen(!createNewUserModalisOpen);🔗 Analysis chain
Line range hint
79-93: Verify modal state managementLet's ensure all modal state changes are working correctly across the component.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential modal state management issues # Look for other modal state manipulations that might need similar fixes # Search for modal state changes echo "Checking modal state manipulations:" rg -n "setAddUserModalIsOpen|setCreateNewUserModalIsOpen" --type tsx # Search for potential conflicting modal states echo "Checking for potential modal state conflicts:" ast-grep --pattern 'useState<boolean>($$$)' --type tsxLength of output: 517
Script:
#!/bin/bash # Let's try a different approach to check modal state management # Search for modal state changes without file type restriction echo "Checking modal state manipulations:" rg -n "setAddUserModalIsOpen|setCreateNewUserModalIsOpen" # Search for useState declarations in the component echo "Checking useState declarations in AddMember.tsx:" rg -A 2 "useState" "src/screens/OrganizationPeople/AddMember.tsx" # Look for other modal-related state management in the component echo "Checking other modal-related code:" rg -A 2 "Modal|modal" "src/screens/OrganizationPeople/AddMember.tsx"Length of output: 4524
🧹 Nitpick comments (1)
src/screens/OrganizationPeople/AddMember.tsx (1)
Line range hint
279-291: Enhance search input handlingWhile the function works correctly, the name splitting logic could be more robust.
Consider this enhancement:
const handleUserModalSearchChange = (e: React.FormEvent): void => { e.preventDefault(); - const [firstName, lastName] = userName.split(' '); + const trimmedName = userName.trim(); + const [firstName, ...lastNameParts] = trimmedName.split(' '); + const lastName = lastNameParts.join(' '); const newFilterData = { - firstName_contains: firstName || '', - lastName_contains: lastName || '', + firstName_contains: trimmedName ? firstName : '', + lastName_contains: lastNameParts.length > 0 ? lastName : '', }; allUsersRefetch({ ...newFilterData, }); };This change:
- Trims whitespace from the input
- Handles multi-word last names correctly
- More explicitly handles empty input cases
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/screens/OrganizationPeople/AddMember.tsx(1 hunks)
🔇 Additional comments (1)
src/screens/OrganizationPeople/AddMember.tsx (1)
79-81: LGTM! This fixes the search value retention issue.
The useEffect hook correctly resets the search input when the modal is toggled, addressing issue #2627.
|
@raggettii Please fix the failing tests |
|
@dhanagopu I only made 1 line change, and the tests are failing that not even from the files that i made changes to |
|
@dhanagopu fixed these failing tests in my PRs #2663 #2662 ..Merging them should fix these ..could you please look into it? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #2682 +/- ##
====================================================
- Coverage 88.37% 84.13% -4.25%
====================================================
Files 295 312 +17
Lines 7046 8130 +1084
Branches 1521 1835 +314
====================================================
+ Hits 6227 6840 +613
- Misses 615 1135 +520
+ Partials 204 155 -49 ☔ View full report in Codecov by Sentry. |
varshith257
merged commit 4b45d85
into
PalisadoesFoundation:develop-postgres
What kind of change does this PR introduce?
It ensures that the text does not retains the previously searched value
Issue Number:
Fixes #2627
Did you add tests for your changes?
No
Snapshots/Videos:
If relevant, did you update the documentation?
Summary
Does this PR introduce a breaking change?
Other information
Have you read the contributing guide?
Yes
Summary by CodeRabbit
New Features
Bug Fixes
Improvements