Upgrade package @testing-library/user-event from 12.8.3 to 14.5.2 #2602
Conversation
WalkthroughThe pull request updates the Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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: 25
🧹 Outside diff range and nitpick comments (45)
src/components/UserPortal/CreateDirectChat/CreateDirectChat.test.tsx (1)
Line range hint
1452-1499: Update second test case to use userEvent APIFor consistency with the first test case and alignment with the package upgrade, this test should also use the userEvent API instead of fireEvent.
Here's how to update the test:
test('create new direct chat', async () => { setItem('userId', '1'); const mock = [ ...GROUP_CHAT_BY_ID_QUERY_MOCK, ...MESSAGE_SENT_TO_CHAT_MOCK, ...UserConnectionListMock, ...CHATS_LIST_MOCK, ...CHAT_BY_ID_QUERY_MOCK, ...CREATE_CHAT_MUTATION_MOCK, ]; render( <MockedProvider addTypename={false} mocks={mock}> <BrowserRouter> <Provider store={store}> <I18nextProvider i18n={i18nForTest}> <Chat /> </I18nextProvider> </Provider> </BrowserRouter> </MockedProvider>, ); await wait(); + const user = userEvent.setup(); const dropdown = await screen.findByTestId('dropdown'); expect(dropdown).toBeInTheDocument(); - fireEvent.click(dropdown); + await user.click(dropdown); const newDirectChatBtn = await screen.findByTestId('newDirectChat'); expect(newDirectChatBtn).toBeInTheDocument(); - fireEvent.click(newDirectChatBtn); + await user.click(newDirectChatBtn); const addBtn = await screen.findAllByTestId('addBtn'); waitFor(() => { expect(addBtn[0]).toBeInTheDocument(); }); - fireEvent.click(addBtn[0]); + await user.click(addBtn[0]); const closeButton = screen.getByRole('button', { name: /close/i }); expect(closeButton).toBeInTheDocument(); - fireEvent.click(closeButton); + await user.click(closeButton); await new Promise(process.nextTick); await wait(); });src/components/UserPasswordUpdate/UserPasswordUpdate.test.tsx (2)
Line range hint
37-45: Review wait helper function usageThe current wait helper with a 5ms timeout might be too short for some async operations. Consider using
waitForfrom @testing-library/react instead.- async function wait(ms = 5): Promise<void> { - await act(() => { - return new Promise((resolve) => { - setTimeout(resolve, ms); - }); - }); - }Replace wait() calls with waitFor:
import { waitFor } from '@testing-library/react'; // Later in tests await waitFor(() => { expect(mockToast.error).toHaveBeenCalledWith(`Password can't be empty`); });
Line range hint
1-8: Update imports to include waitForSince we're using @testing-library/react, we should leverage its built-in utilities for handling async operations.
- import { render, screen } from '@testing-library/react'; + import { render, screen, waitFor } from '@testing-library/react';src/components/Pagination/Pagination.test.tsx (2)
Line range hint
12-19: Consolidate duplicate props declarationThe props object is defined twice with similar structure. Consider consolidating this into a single factory function or base object.
+ const createProps = (direction?: string) => ({ + count: 5, + page: 10, + rowsPerPage: 5, + onPageChange: (): number => { + return 10; + }, + ...(direction && { theme: { direction } }), + }); - const props = { - count: 5, - page: 10, - rowsPerPage: 5, - onPageChange: (): number => { - return 10; - }, - }; test('Component should be rendered properly on rtl', async () => { - // Use createProps() here + const props = createProps(); // ... rest of the test }); - const props = { - count: 5, - page: 10, - rowsPerPage: 5, - onPageChange: (): number => { - return 10; - }, - theme: { direction: 'rtl' }, - }; test('Component should be rendered properly', async () => { + const props = createProps('rtl'); // ... rest of the test });Also applies to: 34-43
Line range hint
19-31: Improve test coverage and assertionsThe current tests simulate user interactions but don't verify their outcomes. Consider:
- Adding specific assertions after each user interaction
- Using more descriptive test names
- Verifying that
onPageChangeis called with correct argumentsExample implementation:
test('should handle next and previous page navigation correctly', async () => { const onPageChange = jest.fn(); const user = userEvent.setup(); render( <BrowserRouter> <Provider store={store}> <Pagination {...createProps()} onPageChange={onPageChange} /> </Provider> </BrowserRouter>, ); await user.click(screen.getByTestId(/nextPage/i)); expect(onPageChange).toHaveBeenCalledWith(expect.any(Object), 11); await user.click(screen.getByTestId(/previousPage/i)); expect(onPageChange).toHaveBeenCalledWith(expect.any(Object), 9); });Also applies to: 44-61
src/components/ProfileDropdown/ProfileDropdown.test.tsx (1)
Line range hint
49-57: Remove duplicate afterEach blockThere are two identical
afterEachblocks performing the same cleanup. This duplication should be removed by keeping only one instance.afterEach(() => { jest.clearAllMocks(); localStorage.clear(); }); -afterEach(() => { - jest.clearAllMocks(); - localStorage.clear(); -});src/screens/MemberDetail/MemberDetail.test.tsx (4)
161-195: Consider extracting form field interactions into a helper functionWhile the implementation is correct, there's repetitive code for form field interactions. Consider creating a helper function to reduce duplication.
const fillFormField = async (user: ReturnType<typeof userEvent.setup>, placeholder: string, value: string) => { await user.clear(screen.getByPlaceholderText(placeholder)); await user.type(screen.getByPlaceholderText(placeholder), value); }; // Usage: await fillFormField(user, /First Name/i, formData.firstName); await fillFormField(user, /Last Name/i, formData.lastName); // etc...
262-266: Consider removing redundant wait callThe wait() call after the click might be unnecessary since userEvent.click() is already async in v14. The subsequent expect statements will naturally wait for the UI to update.
const user = userEvent.setup(); await user.type(screen.getByPlaceholderText(/Address/i), 'random'); await user.type(screen.getByPlaceholderText(/State/i), 'random'); await user.click(screen.getByTestId('resetChangesBtn')); - await wait(); expect(screen.getByPlaceholderText(/First Name/i)).toHaveValue('Aditya');
396-399: Remove unnecessary blank line between related operationsThe click operations are part of the same workflow and should be grouped together without the blank line.
const user = userEvent.setup(); await user.click(screen.getAllByTestId('unassignTagBtn')[0]); - await user.click(screen.getByTestId('unassignTagModalSubmitBtn'));
Line range hint
332-352: Migrate remaining userEvent calls to the new APIFor consistency, the direct userEvent calls in the modal and navigation tests should also be migrated to use the setup-based approach:
test('opens "Events Attended List" modal when View All button is clicked', async () => { renderMemberDetailScreen(link2); await wait(); + const user = userEvent.setup(); const viewAllButton = screen.getByText('View All'); - userEvent.click(viewAllButton); + await user.click(viewAllButton); const modalTitle = await screen.findByText('Events Attended List'); expect(modalTitle).toBeInTheDocument(); }); test('navigates to manage tag screen after clicking manage tag option', async () => { renderMemberDetailScreen(link1); await wait(); await waitFor(() => { expect(screen.getAllByTestId('tagName')[0]).toBeInTheDocument(); }); + const user = userEvent.setup(); - userEvent.click(screen.getAllByTestId('tagName')[0]); + await user.click(screen.getAllByTestId('tagName')[0]); await waitFor(() => { expect(screen.getByTestId('manageTagScreen')).toBeInTheDocument(); }); });src/components/UserPortal/ChatRoom/ChatRoom.tsx (2)
Line range hint
176-181: Optimize scroll behavior and add cleanup for subscriptionThe current scroll effect runs on every render and might cause performance issues. Also, the subscription should be cleaned up when the component unmounts.
Consider these improvements:
- Optimize scroll behavior:
- useEffect(() => { + useEffect(() => { + if (!chat?.messages.length) return; document .getElementById('chat-area') ?.lastElementChild?.scrollIntoView({ block: 'end' }); - }); + }, [chat?.messages.length]);
- Add cleanup for subscription:
const { unsubscribe } = useSubscription(...); useEffect(() => { return () => { if (unsubscribe) { unsubscribe(); } }; }, [unsubscribe]);
Line range hint
1-324: Consider adding error boundaries and loading statesThe component could benefit from better error handling and loading states to improve user experience.
Consider:
- Implementing an error boundary to catch and handle rendering errors
- Adding loading states for the chat data fetching
- Adding error states for failed message sends
- Adding retry mechanisms for failed operations
Would you like me to provide an example implementation of these improvements?
scripts/custom-test-env.js (1)
4-6: Fix TSDoc comment to use proper documentation.The
@pdfmetag is not a standard TSDoc tag. Consider documenting the dependency more clearly./** - * A custom environment to set the TextEncoder and TextDecoder variables, that is required by @pdfme during testing. + * A custom environment to set the TextEncoder and TextDecoder variables that are required + * by the PDF generation library during testing. * Providing a polyfill to the environment for the same */🧰 Tools
🪛 eslint
[error] 5-5: tsdoc-undefined-tag: The TSDoc tag "@pdfme" is not defined in this configuration
(tsdoc/syntax)
src/components/DynamicDropDown/DynamicDropDown.test.tsx (1)
Line range hint
128-137: Consider removing unnecessary act() wrappersThe
act()wrappers aroundfireEventcalls are unnecessary asfireEventalready wraps events inact()internally.Simplify the keyboard event handling:
// Simulate Enter key press - await act(async () => { fireEvent.keyDown(dropdownMenu, { key: 'Enter' }); - }); // Simulate Space key press - await act(async () => { fireEvent.keyDown(dropdownMenu, { key: ' ' }); - });src/components/UserPortal/StartPostModal/StartPostModal.test.tsx (2)
Line range hint
41-48: Consider removing the wait() helper functionWith the new userEvent v14 API, the wait() helper function using act() might be unnecessary as userEvent.setup() already handles proper timing and async behavior.
The wait() function can be removed and tests can rely on userEvent's built-in timing handling:
-async function wait(ms = 100): Promise<void> { - await act(() => { - return new Promise((resolve) => { - setTimeout(resolve, ms); - }); - }); -}
Uncomment and update the success toast test expectations
The commented-out test expectations should be uncommented and updated to match the actual implementation. The success toast is implemented in StartPostModal.tsx with the translation key 'postNowVisibleInFeed', so the test should verify this behavior.
await user.click(screen.getByTestId('createPostBtn')); expect(toast.error).not.toHaveBeenCalledWith(); expect(toast.info).toHaveBeenCalledWith( 'Processing your post. Please wait.', ); await wait(); expect(toast.success).toHaveBeenCalledWith(t('postNowVisibleInFeed'));🔗 Analysis chain
Line range hint
147-154: Consider uncommenting and fixing the success toast test expectationsThe commented-out test expectations for success toast should either be fixed and uncommented or removed if no longer relevant. This improves test coverage and maintains code cleanliness.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if success toast is tested elsewhere rg -l "toast.success.*Your post is now visible" --type tsLength of output: 59
Script:
#!/bin/bash # Let's check for any success toast tests in StartPostModal.test.tsx and related files rg "toast\.success" --type ts -B2 -A2 # Also check for the actual implementation of post creation success handling ast-grep --pattern 'toast.success($$$)'Length of output: 75869
Script:
#!/bin/bash # Let's check the implementation of StartPostModal.tsx to understand the success toast usage rg "postNowVisibleInFeed|Your post is now visible" --type ts -B2 -A2 # Also check for any post creation success handling in StartPostModal.tsx cat src/components/UserPortal/StartPostModal/StartPostModal.tsxLength of output: 6068
src/components/EventRegistrantsModal/AddOnSpotAttendee.test.tsx (1)
Line range hint
173-175: Update to new userEvent APIThis test still uses the old userEvent API. Update it to use userEvent.setup() for consistency with other tests.
-userEvent.type(screen.getByLabelText('First Name'), 'John'); -userEvent.type(screen.getByLabelText('Last Name'), 'Doe'); +const user = userEvent.setup(); +await user.type(screen.getByLabelText('First Name'), 'John'); +await user.type(screen.getByLabelText('Last Name'), 'Doe');src/components/OrgSettings/AgendaItemCategories/AgendaCategoryUpdateModal.test.tsx (1)
Line range hint
83-123: Consider updating form interaction tests to use userEvent.The form interaction tests still use
fireEvent. For consistency and better simulation of real user interactions, consider updating these to useuserEvent.Here's how to update the form state test:
- fireEvent.change(nameInput, { - target: { value: 'New name' }, - }); - fireEvent.change(descriptionInput, { - target: { value: 'New description' }, - }); + const user = userEvent.setup(); + await user.type(nameInput, 'New name'); + await user.type(descriptionInput, 'New description');Similarly, update the form submission test:
- fireEvent.submit(screen.getByTestId('editAgendaCategoryBtn')); + const user = userEvent.setup(); + await user.click(screen.getByTestId('editAgendaCategoryBtn'));src/components/UserPortal/UserNavbar/UserNavbar.test.tsx (2)
Line range hint
16-22: Consider removing custom wait function.The custom
waitfunction might be redundant asuserEventfrom v14 handles timing automatically. Consider removing it and relying onuserEvent's built-in timing mechanisms.
Line range hint
73-179: Consider reducing test duplication.The language switching tests have significant duplication. Consider using a test.each pattern to make the tests more maintainable.
Example refactor:
const languages = [ { index: 0, code: 'en' }, { index: 1, code: 'fr' }, { index: 2, code: 'hi' }, { index: 3, code: 'sp' }, { index: 4, code: 'zh' } ]; test.each(languages)('The language is switched to $code', async ({ index, code }) => { render( <MockedProvider addTypename={false} link={link}> <BrowserRouter> <Provider store={store}> <I18nextProvider i18n={i18nForTest}> <UserNavbar /> </I18nextProvider> </Provider> </BrowserRouter> </MockedProvider> ); const user = userEvent.setup(); await user.click(screen.getByTestId('languageIcon')); await user.click(screen.getByTestId(`changeLanguageBtn${index}`)); expect(cookies.get('i18next')).toBe(code); });src/components/EventManagement/EventAgendaItems/EventAgendaItems.test.tsx (1)
143-156: Consider adding error boundaries to modal interaction testThe conversion to userEvent.setup() and async/await is correct. However, consider adding error handling for better test resilience:
const user = userEvent.setup(); -await user.click(screen.getByTestId('createAgendaItemBtn')); +await user.click(screen.getByTestId('createAgendaItemBtn')).catch(error => { + console.error('Failed to click create button:', error); + throw error; +}); await waitFor(() => { return expect( screen.findByTestId('createAgendaItemModalCloseBtn'), ).resolves.toBeInTheDocument(); }); -await user.click(screen.getByTestId('createAgendaItemModalCloseBtn')); +await user.click(screen.getByTestId('createAgendaItemModalCloseBtn')).catch(error => { + console.error('Failed to click close button:', error); + throw error; +});src/components/AddOn/core/AddOnRegister/AddOnRegister.test.tsx (1)
118-125: Consider extracting repeated userEvent setup patternThe implementation of userEvent.setup() is correct, but there's repetition across test cases. Consider extracting the setup into a test utility:
const setupUserAndFillForm = async (pluginData: typeof pluginData) => { const user = userEvent.setup(); await user.click(screen.getByRole('button', { name: /Add New/i })); await wait(100); await user.type(screen.getByTestId('pluginName'), pluginData.pluginName); await user.type(screen.getByTestId('pluginCreatedBy'), pluginData.pluginCreatedBy); await user.type(screen.getByTestId('pluginDesc'), pluginData.pluginDesc); return user; };This would simplify your test cases and make them more maintainable.
Also applies to: 147-157, 179-189
src/components/OrgSettings/ActionItemCategories/CategoryModal.test.tsx (1)
Line range hint
1-1: Consider implementing a shared test utilities fileGiven the common patterns across these test files (userEvent setup, wait utilities, form filling), consider creating a shared test utilities file to maintain consistency and reduce duplication across the test suite.
This could include:
- Standard userEvent setup with error handling
- Common wait utilities
- Shared form interaction patterns
- Type-safe test helpers
This would make the tests more maintainable and ensure consistent practices across the codebase.
src/screens/UserPortal/People/People.test.tsx (2)
137-153: Consider enhancing test reliability with more specific assertionsThe test could be improved by:
- Using
waitForinstead of the customwaitfunction- Making the assertion more specific by checking for exact number of elements
- await wait(); - expect(screen.queryAllByText('Noble Mittal')).not.toBe([]); + await waitFor(() => { + expect(screen.getAllByText('Noble Mittal')).toHaveLength(2); + });
Line range hint
215-239: Improve test reliability with proper async handlingThe mode change test should use
userEvent.setup()and properly await all user interactions.- userEvent.click(screen.getByTestId('modeChangeBtn')); - await wait(); - userEvent.click(screen.getByTestId('modeBtn1')); - await wait(); + const user = userEvent.setup(); + await user.click(screen.getByTestId('modeChangeBtn')); + await user.click(screen.getByTestId('modeBtn1'));src/components/OrgSettings/AgendaItemCategories/OrganizationAgendaCategory.test.tsx (1)
169-187: Use consistent event handling patternsFor consistency with other user interactions, consider using
userEventfor form submission instead offireEvent.- fireEvent.submit(screen.getByTestId('createAgendaCategoryFormSubmitBtn')); + await user.click(screen.getByTestId('createAgendaCategoryFormSubmitBtn'));src/screens/UserPortal/Volunteer/Actions/Actions.test.tsx (1)
Line range hint
114-134: Modernize sorting test to use userEventThe sorting functionality test should use
userEvent.setup()for consistency with other tests.+ const user = userEvent.setup(); - fireEvent.click(sortBtn); + await user.click(sortBtn); - fireEvent.click(dueDateDESC); + await user.click(dueDateDESC); let assigneeName = await screen.findAllByTestId('assigneeName'); expect(assigneeName[0]).toHaveTextContent('Group 1'); // Sort by dueDate_ASC sortBtn = await screen.findByTestId('sort'); expect(sortBtn).toBeInTheDocument(); - fireEvent.click(sortBtn); + await user.click(sortBtn); const dueDateASC = await screen.findByTestId('dueDate_ASC'); expect(dueDateASC).toBeInTheDocument(); - fireEvent.click(dueDateASC); + await user.click(dueDateASC);src/components/OrgSettings/ActionItemCategories/OrgActionItemCategories.test.tsx (1)
31-37: Consider extracting the wait utility to a shared test helperThe wait utility function is duplicated across multiple test files. Consider moving it to a shared test utility file to promote code reuse and maintainability.
Create a new file
src/utils/testUtils.ts:import { act } from '@testing-library/react'; export async function wait(ms = 100): Promise<void> { await act(() => { return new Promise((resolve) => { setTimeout(resolve, ms); }); }); }src/components/AddPeopleToTag/AddPeopleToTag.test.tsx (1)
296-309: LGTM! Good adoption of userEvent.setup()The changes correctly implement the new userEvent.setup() pattern from @testing-library/user-event v14, improving the reliability of async interaction testing. The await statements are properly used with user interactions.
Consider applying the same userEvent.setup() pattern to other tests in this file that still use the old userEvent.click() syntax, such as the test for selecting and deselecting members. This would maintain consistency across all tests.
src/screens/EventVolunteers/VolunteerGroups/VolunteerGroupModal.test.tsx (1)
192-192: Consider consistent event handling approach across test files.While using
fireEvent.submit()is semantically correct for form submissions, it differs from theuserEvent.setup()pattern used in other test files. Consider standardizing the approach:Option 1 (matches other files):
-fireEvent.submit(submitBtn); +const user = userEvent.setup(); +await user.click(submitBtn);Option 2 (current approach):
-userEvent.click(submitBtn); +fireEvent.submit(submitBtn);Also applies to: 242-242, 270-270, 300-300, 343-343
src/screens/FundCampaignPledge/FundCampaignPledge.test.tsx (1)
Line range hint
1-450: Update userEvent implementation to use setup patternThe test file is still using the older userEvent.click() pattern. To maintain consistency with the @testing-library/user-event v14 upgrade, update the userEvent calls to use the new setup pattern.
Example refactor:
- userEvent.click(addPledgeBtn); + const user = userEvent.setup(); + await user.click(addPledgeBtn);This change should be applied to all userEvent calls in the file for:
- addPledgeBtn click
- editPledgeBtn click
- deletePledgeBtn click
- moreContainer click
- filter clicks
src/components/AgendaCategory/AgendaCategoryContainer.test.tsx (1)
Line range hint
255-316: Complete the migration to userEvent.setup()While some test cases have been updated to use the new userEvent.setup() pattern, there are still instances using the older userEvent.click() pattern. For consistency, all userEvent calls should be updated.
Update the remaining instances:
- userEvent.click(screen.getAllByTestId('previewAgendaCategoryModalBtn')[0]); + const user = userEvent.setup(); + await user.click(screen.getAllByTestId('previewAgendaCategoryModalBtn')[0]); - userEvent.click(screen.getByTestId('deleteAgendaCategoryModalBtn')); + await user.click(screen.getByTestId('deleteAgendaCategoryModalBtn')); - userEvent.click(screen.getByTestId('deleteAgendaCategoryBtn')); + await user.click(screen.getByTestId('deleteAgendaCategoryBtn'));This change should be applied to all remaining userEvent.click() calls in the file for complete consistency with @testing-library/user-event v14.
src/screens/OrgList/OrgList.test.tsx (2)
313-317: Consider adding error handling for file uploadWhile the implementation is correct, consider adding error handling for the file upload operation to make the tests more robust.
await user.upload(displayImage, formData.image); +await waitFor(() => { + expect(displayImage.files[0]).toBeTruthy(); + expect(displayImage.files[0].name).toBe('hello.png'); +}); fireEvent.submit(screen.getByTestId(/submitOrganizationForm/i));
Line range hint
419-432: Clean up commented code and standardize event handlingTwo issues to address:
- Remove commented-out code that's no longer needed
- Consistently use userEvent instead of mixing with fireEvent
- // await act(async () => { - // await new Promise((resolve) => setTimeout(resolve, 1000)); - // }); await waitFor(() => expect( screen.queryByText(/Congratulation the Organization is created/i), ).toBeInTheDocument(), ); await waitFor(() => { screen.findByTestId(/pluginNotificationHeader/i); }); - // userEvent.click(screen.getByTestId(/enableEverythingForm/i)); await user.click(screen.getByTestId(/enableEverythingForm/i));src/components/UserPortal/PostCard/PostCard.test.tsx (1)
494-496: Consider adding wait between sequential operationsWhile the migration to userEvent v14 is correct, consider adding a wait between the view and like operations to prevent potential race conditions.
const user = userEvent.setup(); await user.click(screen.getByTestId('viewPostBtn')); + await wait(); await user.click(screen.getByTestId('likePostBtn'));src/components/Advertisements/Advertisements.test.tsx (1)
Line range hint
384-410: Migrate from fireEvent to userEvent for consistencyThe test still uses fireEvent while the PR's objective is to upgrade to userEvent v14. For consistency and better simulation of real user interactions, consider migrating these operations to userEvent.
- fireEvent.click(screen.getByText('Create Advertisement')); - fireEvent.change(screen.getByLabelText('Enter name of Advertisement'), { - target: { value: 'Cookie Shop' }, - }); + const user = userEvent.setup(); + await user.click(screen.getByText('Create Advertisement')); + await user.type(screen.getByLabelText('Enter name of Advertisement'), 'Cookie Shop'); const mediaFile = new File(['media content'], 'test.png', { type: 'image/png', }); - fireEvent.change(screen.getByLabelText('Select type of Advertisement'), { - target: { value: 'POPUP' }, - }); - fireEvent.change(screen.getByLabelText('Select Start Date'), { - target: { value: '2023-01-01' }, - }); - fireEvent.change(screen.getByLabelText('Select End Date'), { - target: { value: '2023-02-02' }, - }); + await user.selectOptions(screen.getByLabelText('Select type of Advertisement'), 'POPUP'); + await user.type(screen.getByLabelText('Select Start Date'), '2023-01-01'); + await user.type(screen.getByLabelText('Select End Date'), '2023-02-02'); - fireEvent.click(screen.getByTestId('addonregister')); + await user.click(screen.getByTestId('addonregister'));src/screens/OrgPost/OrgPost.test.tsx (1)
Line range hint
1-1: Consider establishing testing guidelines for event handlingWhile the PR successfully upgrades to @testing-library/user-event v14.5.2, there's inconsistent usage of event handling methods across test files. Consider establishing testing guidelines that:
- Standardize the use of userEvent over fireEvent for better simulation of real user interactions
- Define patterns for handling async operations and potential race conditions
- Document best practices for test setup and cleanup
This will help maintain consistency across the test suite and prevent potential issues with async operations.
src/screens/UserPortal/Settings/Settings.test.tsx (2)
239-242: Consider using consistent event handling methodsWhile the userEvent.setup() implementation is correct, mixing userEvent and fireEvent could lead to inconsistent behavior. Consider using user.click() instead of fireEvent.click() for consistency.
const user = userEvent.setup(); await user.type(screen.getByTestId('inputAddress'), 'random'); await wait(); -fireEvent.click(screen.getByTestId('resetChangesBtn')); +await user.click(screen.getByTestId('resetChangesBtn'));
273-276: Consider using consistent event handling methodsSimilar to the previous test case, consider using user.click() instead of fireEvent.click() for consistency with the userEvent.setup() pattern.
const user = userEvent.setup(); await user.type(screen.getByTestId('inputAddress'), 'random'); await wait(); -fireEvent.click(screen.getByTestId('resetChangesBtn')); +await user.click(screen.getByTestId('resetChangesBtn'));src/screens/ManageTag/ManageTag.test.tsx (1)
132-136: Improve error handling in async operationsThe error handling for async operations could be more robust.
Consider adding timeout and error messages to findByTestId calls:
- userEvent.click(await screen.findByTestId('addPeopleToTagBtn')); + userEvent.click(await screen.findByTestId('addPeopleToTagBtn', { + timeout: 2000, + })); - expect(await screen.findByTestId('addPeopleToTagModal')).toBeInTheDocument(); + expect( + await screen.findByTestId('addPeopleToTagModal', { + timeout: 2000, + }) + ).toBeInTheDocument();Also applies to: 138-138
src/components/RecurrenceOptions/CustomRecurrence.test.tsx (1)
245-250: Consider simplifying the forEach loops with Promise.all.The current implementation might lead to race conditions as async operations in forEach aren't properly coordinated.
- weekDaysOptions.forEach(async (weekDay) => { - await user.click(weekDay); - }); - - weekDaysOptions.forEach(async (weekDay) => { - await user.click(weekDay); - }); + await Promise.all(weekDaysOptions.map((weekDay) => user.click(weekDay))); + await Promise.all(weekDaysOptions.map((weekDay) => user.click(weekDay)));src/components/OrgPostCard/OrgPostCard.test.tsx (4)
Line range hint
174-180: Consider using userEvent for all interactionsThe test still uses
fireEventfor toggle button clicks. For consistency and better simulation of real user interactions, consider migrating these to use the setup'd userEvent instance.- fireEvent.click(toggleButton); - expect(toggleButton).toHaveTextContent('hide'); - fireEvent.click(toggleButton); + await user.click(toggleButton); + expect(toggleButton).toHaveTextContent('hide'); + await user.click(toggleButton);
Line range hint
232-243: Consider using userEvent for form interactionsSeveral test cases still use
fireEvent.changefor form interactions. userEvent provides better simulation of real user input behavior.- fireEvent.change(getByTestId('updateTitle'), { - target: { value: 'updated title' }, - }); + await user.type(getByTestId('updateTitle'), 'updated title');Also applies to: 432-434, 689-697, 715-723
Line range hint
760-785: Consider updating mouse events to use userEventThe video playback test still uses
fireEventfor mouse events. Consider updating to use userEvent's hover methods for better interaction simulation.- fireEvent.mouseEnter(card); + await user.hover(card); - fireEvent.mouseLeave(card); + await user.unhover(card);
Line range hint
1-137: Consider adding type coverage for test dataThe mock data and props are using implicit typing. Consider adding explicit TypeScript interfaces for better type safety and documentation.
interface PostCardProps { id: string; postID: string; postTitle: string; postInfo: string; postAuthor: string; postPhoto: string; postVideo: string; pinned: boolean; } const props: PostCardProps = { id: '12', postID: '123', // ... rest of the props };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (65)
.eslintignore(1 hunks).github/workflows/pull-request.yml(9 hunks)package.json(2 hunks)scripts/custom-test-env.js(1 hunks)src/components/AddOn/core/AddOnRegister/AddOnRegister.test.tsx(3 hunks)src/components/AddOn/core/AddOnStore/AddOnStore.test.tsx(2 hunks)src/components/AddPeopleToTag/AddPeopleToTag.test.tsx(1 hunks)src/components/Advertisements/Advertisements.test.tsx(2 hunks)src/components/Advertisements/core/AdvertisementRegister/AdvertisementRegister.test.tsx(1 hunks)src/components/AgendaCategory/AgendaCategoryContainer.test.tsx(2 hunks)src/components/AgendaItems/AgendaItemsContainer.test.tsx(14 hunks)src/components/DynamicDropDown/DynamicDropDown.test.tsx(3 hunks)src/components/EventListCard/EventListCard.test.tsx(38 hunks)src/components/EventManagement/EventAgendaItems/EventAgendaItems.test.tsx(3 hunks)src/components/EventManagement/EventAttendance/EventAttendance.test.tsx(1 hunks)src/components/EventManagement/EventAttendance/EventStatistics.test.tsx(1 hunks)src/components/EventRegistrantsModal/AddOnSpotAttendee.test.tsx(2 hunks)src/components/LeftDrawer/LeftDrawer.test.tsx(2 hunks)src/components/LeftDrawerOrg/LeftDrawerOrg.test.tsx(1 hunks)src/components/OrgPostCard/OrgPostCard.test.tsx(28 hunks)src/components/OrgSettings/ActionItemCategories/CategoryModal.test.tsx(1 hunks)src/components/OrgSettings/ActionItemCategories/OrgActionItemCategories.test.tsx(4 hunks)src/components/OrgSettings/AgendaItemCategories/AgendaCategoryUpdateModal.test.tsx(2 hunks)src/components/OrgSettings/AgendaItemCategories/OrganizationAgendaCategory.test.tsx(4 hunks)src/components/OrgSettings/General/OrgUpdate/OrgUpdate.test.tsx(1 hunks)src/components/Pagination/Pagination.test.tsx(3 hunks)src/components/ProfileDropdown/ProfileDropdown.test.tsx(3 hunks)src/components/RecurrenceOptions/CustomRecurrence.test.tsx(17 hunks)src/components/RecurrenceOptions/RecurrenceOptions.test.tsx(10 hunks)src/components/UpdateSession/UpdateSession.test.tsx(7 hunks)src/components/UserPasswordUpdate/UserPasswordUpdate.test.tsx(2 hunks)src/components/UserPortal/ChatRoom/ChatRoom.tsx(1 hunks)src/components/UserPortal/CreateDirectChat/CreateDirectChat.test.tsx(3 hunks)src/components/UserPortal/OrganizationNavbar/OrganizationNavbar.test.tsx(6 hunks)src/components/UserPortal/PostCard/PostCard.test.tsx(9 hunks)src/components/UserPortal/Register/Register.test.tsx(4 hunks)src/components/UserPortal/StartPostModal/StartPostModal.test.tsx(2 hunks)src/components/UserPortal/UserNavbar/UserNavbar.test.tsx(7 hunks)src/components/UserPortal/UserSidebarOrg/UserSidebarOrg.test.tsx(1 hunks)src/screens/BlockUser/BlockUser.test.tsx(7 hunks)src/screens/CommunityProfile/CommunityProfile.test.tsx(2 hunks)src/screens/EventManagement/EventManagement.test.tsx(3 hunks)src/screens/EventVolunteers/VolunteerGroups/VolunteerGroupModal.test.tsx(5 hunks)src/screens/ForgotPassword/ForgotPassword.test.tsx(9 hunks)src/screens/FundCampaignPledge/FundCampaignPledge.test.tsx(1 hunks)src/screens/LoginPage/LoginPage.test.tsx(8 hunks)src/screens/ManageTag/ManageTag.test.tsx(20 hunks)src/screens/MemberDetail/MemberDetail.test.tsx(3 hunks)src/screens/OrgList/OrgList.test.tsx(7 hunks)src/screens/OrgPost/OrgPost.test.tsx(16 hunks)src/screens/OrganizationActionItems/ItemModal.test.tsx(3 hunks)src/screens/OrganizationActionItems/OrganizationActionItems.test.tsx(7 hunks)src/screens/OrganizationEvents/OrganizationEvents.test.tsx(9 hunks)src/screens/OrganizationPeople/OrganizationPeople.test.tsx(20 hunks)src/screens/OrganizationTags/OrganizationTags.test.tsx(1 hunks)src/screens/SubTags/SubTags.test.tsx(3 hunks)src/screens/UserPortal/Donate/Donate.test.tsx(2 hunks)src/screens/UserPortal/Events/Events.test.tsx(7 hunks)src/screens/UserPortal/People/People.test.tsx(3 hunks)src/screens/UserPortal/Posts/Posts.test.tsx(4 hunks)src/screens/UserPortal/Settings/Settings.test.tsx(2 hunks)src/screens/UserPortal/Volunteer/Actions/Actions.test.tsx(3 hunks)src/screens/UserPortal/Volunteer/VolunteerManagement.test.tsx(1 hunks)src/setupTests.ts(1 hunks)vitest.config.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- .eslintignore
🧰 Additional context used
📓 Learnings (4)
src/screens/OrganizationActionItems/OrganizationActionItems.test.tsx (1)
Learnt from: Chaitanya1672
PR: PalisadoesFoundation/talawa-admin#2049
File: src/screens/OrganizationActionItems/ActionItemUpdateModal.tsx:112-138
Timestamp: 2024-11-12T10:40:58.654Z
Learning: The `istanbul ignore next` comments in the `ActionItemUpdateModal.tsx` file were added as part of a commit that introduced tests for the `ActionItemUpdateModal` component. Removing these comments and writing tests to cover the previously ignored lines is recommended to ensure code integrity and improve test coverage.
src/screens/OrganizationActionItems/ItemModal.test.tsx (1)
Learnt from: Chaitanya1672
PR: PalisadoesFoundation/talawa-admin#2049
File: src/screens/OrganizationActionItems/ActionItemUpdateModal.tsx:112-138
Timestamp: 2024-11-12T10:40:58.654Z
Learning: The `istanbul ignore next` comments in the `ActionItemUpdateModal.tsx` file were added as part of a commit that introduced tests for the `ActionItemUpdateModal` component. Removing these comments and writing tests to cover the previously ignored lines is recommended to ensure code integrity and improve test coverage.
src/components/LeftDrawerOrg/LeftDrawerOrg.test.tsx (1)
Learnt from: Doraemon012
PR: PalisadoesFoundation/talawa-admin#1988
File: src/components/LeftDrawerOrg/LeftDrawerOrg.test.tsx:282-282
Timestamp: 2024-11-12T10:40:58.654Z
Learning: In the test 'Component should be rendered properly' within 'Testing LeftDrawerOrg component for SUPERADMIN', setting 'SuperAdmin' to false is intentional.
src/screens/OrganizationTags/OrganizationTags.test.tsx (1)
Learnt from: meetulr
PR: PalisadoesFoundation/talawa-admin#2398
File: src/screens/OrganizationTags/OrganizationTags.test.tsx:184-0
Timestamp: 2024-11-12T10:40:58.655Z
Learning: In the `talawa-admin` project, it's acceptable for test cases in `src/screens/OrganizationTags/OrganizationTags.test.tsx` to test multiple behaviors within a single test function without needing to split them into smaller, focused tests.
🪛 eslint
scripts/custom-test-env.js
[error] 5-5: tsdoc-undefined-tag: The TSDoc tag "@pdfme" is not defined in this configuration
(tsdoc/syntax)
🪛 Biome (1.9.4)
vitest.config.ts
[error] 1-1: Expected a statement but instead found '<<<<<<< HEAD'.
Expected a statement here.
(parse)
[error] 2-3: Expected a statement but instead found '======='.
Expected a statement here.
(parse)
[error] 4-5: Expected a statement but instead found '>>>>>>> c2630ae'.
Expected a statement here.
(parse)
[error] 4-4: Shouldn't redeclare 'defineConfig'. Consider to delete it or rename it.
'defineConfig' is defined here:
(lint/suspicious/noRedeclare)
[error] 22-22: Expected an expression but instead found '<<'.
Expected an expression here.
(parse)
[error] 22-22: Expected an expression but instead found '<<'.
Expected an expression here.
(parse)
[error] 22-23: expected > but instead found ===
Remove ===
(parse)
[error] 23-23: Expected an expression but instead found '==='.
Expected an expression here.
(parse)
[error] 23-23: Expected an expression but instead found '='.
Expected an expression here.
(parse)
[error] 24-24: expected , but instead found :
Remove :
(parse)
[error] 24-25: Expected a property, a shorthand property, a getter, a setter, or a method but instead found '>'.
Expected a property, a shorthand property, a getter, a setter, or a method here.
(parse)
[error] 25-25: Expected an expression but instead found '>>>'.
Expected an expression here.
(parse)
[error] 25-25: Expected an expression but instead found '>'.
Expected an expression here.
(parse)
🪛 actionlint (1.7.4)
.github/workflows/pull-request.yml
61-61: could not parse as YAML: yaml: line 61: could not find expected ':'
(syntax-check)
🪛 yamllint (1.35.1)
.github/workflows/pull-request.yml
[error] 62-62: syntax error: could not find expected ':'
(syntax)
[error] 433-433: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (55)
src/components/UserPortal/CreateDirectChat/CreateDirectChat.test.tsx (2)
23-23: LGTM: Import statement aligns with package upgrade
The import of userEvent from @testing-library/user-event is correctly added.
Line range hint 1425-1449: LGTM: Correct implementation of userEvent API
The changes properly implement the new userEvent API with correct async/await usage, replacing fireEvent with userEvent.setup() and await user.click()/type().
src/components/ProfileDropdown/ProfileDropdown.test.tsx (3)
122-124: LGTM! Properly updated to use new userEvent.setup() pattern
The changes correctly implement the new user interaction pattern from @testing-library/user-event v14, including:
- Using
userEvent.setup()to create a user instance - Properly awaiting user interactions
138-140: LGTM! Consistent implementation of new userEvent pattern
The changes maintain consistency with the new user interaction pattern, properly implementing async/await with userEvent.setup().
Line range hint 1-144: Verify all user interactions are updated
Let's ensure all user interactions in this file have been updated to the new pattern.
✅ Verification successful
All user interactions are properly updated to the new pattern
Based on the verification results, all user interactions in the test file are following the correct pattern:
- All userEvent instances use the new
setup()pattern - All user interactions are properly
awaited - The two test cases with user interactions ("logout functionality" and "member screen") are correctly marked as
async - Each interaction chain (click on dropdown followed by specific button) follows the correct async pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining old userEvent usage patterns that might have been missed
rg -l "userEvent\.(click|type|keyboard|hover)" src/components/ProfileDropdown/ProfileDropdown.test.tsx
# Search for potential missed async/await on user interactions
rg "user\.(click|type|keyboard|hover)" src/components/ProfileDropdown/ProfileDropdown.test.tsx | grep -v "await"
Length of output: 217
Script:
#!/bin/bash
# Let's try a different approach to check user event patterns
# First, let's see all userEvent related lines for a complete picture
rg "userEvent" -A 2 src/components/ProfileDropdown/ProfileDropdown.test.tsx
# Then check all user interaction patterns
rg "user\." -A 1 src/components/ProfileDropdown/ProfileDropdown.test.tsx
# Finally, let's check for any test cases that might involve user interactions
rg "test\(" -A 3 src/components/ProfileDropdown/ProfileDropdown.test.tsx
Length of output: 1721
src/components/LeftDrawer/LeftDrawer.test.tsx (1)
2-2: LGTM! Good addition of waitFor utility
The addition of waitFor from @testing-library/react is appropriate for handling asynchronous assertions in the tests.
src/screens/MemberDetail/MemberDetail.test.tsx (2)
159-160: LGTM! Correct usage of userEvent.setup()
The migration to the new userEvent API is implemented correctly. The setup() method creates an instance that supports the new promise-based API.
202-202: LGTM! Correct async click implementation
The click event is properly awaited, following the new userEvent API requirements.
scripts/custom-test-env.js (1)
2-2: LGTM! Environment setup changes are correct.
The changes to use TestEnvironment from jest-environment-jsdom align with the latest version (29.7.0) and follow the correct implementation pattern.
Also applies to: 8-8
src/setupTests.ts (1)
9-9: LGTM! Improved console mocking approach.
The changes simplify console mocking by using jest.fn() instead of throwing errors. This is a better approach as it:
- Allows tracking console calls during tests
- Provides more flexibility in test assertions
- Aligns with modern Jest practices
Let's verify no other files depend on the old error-throwing behavior:
Also applies to: 11-12
✅ Verification successful
Removal of util.format import and console mock changes are safe
The verification shows no test files in the codebase that depend on console.error/warn throwing errors or have expectations around specific console behavior. The simplified mocking approach using jest.fn() is safe to implement.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test files that might expect console errors to throw
rg -l "expect.*console\.(error|warn).*toThrow" "src/**/*.test.{ts,tsx}"
Length of output: 136
Script:
#!/bin/bash
# Let's try searching without the glob pattern which might not work in the sandbox
# Search for test files that might expect console errors to throw
fd "test\.(ts|tsx)$" | xargs rg -l "expect.*console\.(error|warn).*toThrow"
# Also search for any test assertions related to console.error/warn
fd "test\.(ts|tsx)$" | xargs rg "expect.*console\.(error|warn)" -A 2
Length of output: 148
vitest.config.ts (1)
23-23: Verify the existence of vitest setup file.
Ensure that 'vitest.setup.ts' exists and contains the necessary test setup configuration.
✅ Verification successful
Setup file exists and contains the required testing library import
The vitest.setup.ts file exists and includes the import for @testing-library/jest-dom, which is a common testing utility that extends Jest's matchers for DOM testing.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the setup file exists
if [ -f "vitest.setup.ts" ]; then
echo "Setup file exists"
# Show its contents
cat "vitest.setup.ts"
else
echo "Setup file is missing"
fi
Length of output: 131
🧰 Tools
🪛 Biome (1.9.4)
[error] 22-23: expected > but instead found ===
Remove ===
(parse)
[error] 23-23: Expected an expression but instead found '==='.
Expected an expression here.
(parse)
[error] 23-23: Expected an expression but instead found '='.
Expected an expression here.
(parse)
[error] 18-23: Invalid assignment to { include: ['src/**/*.spec.{js,jsx,ts,tsx}'], globals: true, environment: 'jsdom', <<<<<<< HEAD ======
This expression cannot be assigned to
(parse)
src/components/DynamicDropDown/DynamicDropDown.test.tsx (1)
45-50: LGTM! Consistent usage of userEvent.setup()
The migration to userEvent.setup() has been implemented consistently across all click interactions in this file.
Also applies to: 83-87, 122-123
src/components/OrgSettings/AgendaItemCategories/AgendaCategoryUpdateModal.test.tsx (1)
Line range hint 56-79: LGTM! Proper implementation of async userEvent.
The transition to using userEvent.setup() for handling the close button click is correctly implemented with proper async/await syntax.
package.json (1)
121-121: LGTM! Proper version upgrades for testing dependencies.
The upgrades to testing libraries are correctly implemented:
- @testing-library/user-event: ^14.5.2
- jest: ^29.7.0
- jest-environment-jsdom: ^29.7.0
These versions are stable and compatible with each other.
Also applies to: 150-151
src/components/UserPortal/UserNavbar/UserNavbar.test.tsx (1)
73-76: LGTM! Consistent implementation of async userEvent across all tests.
The transition to userEvent.setup() is properly implemented with correct async/await syntax across all test cases.
Also applies to: 98-101, 123-126, 148-150, 172-175, 197-198, 218-220
src/components/EventManagement/EventAgendaItems/EventAgendaItems.test.tsx (1)
40-44: LGTM! Well-implemented type checking for console.error mock
The type checking for the console.error message parameter is a good practice, ensuring type safety while filtering React 18 deprecation warnings.
src/screens/UserPortal/Volunteer/Actions/Actions.test.tsx (2)
151-158: LGTM! Well-implemented search functionality tests
The search functionality tests demonstrate good practices:
- Proper use of
userEvent.setup() - Correct async/await pattern
- Appropriate debounce handling
Also applies to: 172-180
208-223: LGTM! Well-structured modal interaction tests
The modal interaction tests demonstrate good practices with proper async handling and clear assertions.
src/components/EventManagement/EventAttendance/EventStatistics.test.tsx (1)
175-185: LGTM! Good implementation of button state testing
The changes not only adopt the new userEvent.setup() pattern but also improve test quality by:
- Properly awaiting user interactions
- Adding explicit assertions for button states
- Using proper class-based assertions for visual state verification
src/components/UpdateSession/UpdateSession.test.tsx (4)
76-76: LGTM! Simplified console warning mock
Good cleanup of the console warning mock implementation.
95-99: LGTM! Proper implementation of slider interactions
The change to use fireEvent for slider interactions is correct as these are low-level DOM events. The implementation properly simulates:
- Mouse down for initial interaction
- Mouse up for completing the interaction
- Correct clientX values for different slider positions
Also applies to: 116-120, 136-140
158-168: LGTM! Comprehensive slider drag simulation
Excellent implementation of slider drag interaction that properly simulates:
- Initial mouse down with button state
- Mouse movement with maintained button state
- Final mouse up
222-222: LGTM! Correct event type for form submission
Good change to use fireEvent.submit instead of click for form submission, as it better represents the actual form submission event.
src/screens/UserPortal/Donate/Donate.test.tsx (2)
307-309: LGTM! Correct implementation of userEvent.setup()
The changes correctly implement the new async pattern from @testing-library/user-event@14.
333-335: LGTM! Consistent implementation of userEvent.setup()
The changes maintain consistency with the async pattern throughout the test file.
src/screens/SubTags/SubTags.test.tsx (3)
285-286: LGTM! Proper async handling for sort functionality
The changes correctly implement the async pattern for testing sort interactions.
Also applies to: 291-291
303-303: LGTM! Consistent async handling
The changes maintain consistency in implementing the async pattern.
Also applies to: 308-308
356-364: LGTM! Well-structured async operations
The changes properly implement sequential async operations using userEvent.setup().
src/screens/CommunityProfile/CommunityProfile.test.tsx (2)
262-262: LGTM! Proper async handling of button click.
The click event is correctly awaited using the setup pattern.
233-244: LGTM! User event handling updated correctly.
The changes properly implement the new userEvent.setup() pattern and correctly await all user interactions.
Let's verify all userEvent migrations in test files:
src/screens/ForgotPassword/ForgotPassword.test.tsx (2)
3-9: LGTM: Clean import organization
The imports are well-organized and include all necessary testing utilities.
170-171: LGTM: Consistent implementation of userEvent v14 patterns
The changes consistently implement the new userEvent v14 patterns across all test cases:
- Using
userEvent.setup() - Properly awaiting all user interactions
- Maintaining test case integrity while updating the syntax
Also applies to: 176-176, 204-205, 210-210, 213-218, 249-250, 255-255, 258-263
src/screens/OrganizationActionItems/OrganizationActionItems.test.tsx (1)
220-221: LGTM: Comprehensive update to userEvent v14 patterns
The changes systematically update all user interactions to use the new userEvent v14 patterns:
- Consistent use of
userEvent.setup() - All user interactions are properly awaited
- Test logic and assertions are preserved while updating the interaction syntax
Also applies to: 226-226, 237-238, 243-243, 254-255, 260-260, 271-272, 277-277, 288-289, 294-294, 306-307, 312-312, 317-317, 332-333, 338-338, 343-343
src/components/UserPortal/OrganizationNavbar/OrganizationNavbar.test.tsx (1)
237-238: LGTM! Correct implementation of userEvent.setup()
The changes correctly implement the new userEvent pattern from @testing-library/user-event v14. All user interactions are properly awaited, and the implementation is consistent across all test cases.
Also applies to: 259-262, 290-293, 315-318, 340-343, 365-368
src/components/AgendaItems/AgendaItemsContainer.test.tsx (2)
34-41: LGTM! Improved type safety in console error spying.
The type check for the message parameter before checking its content is a good practice that prevents potential runtime errors.
124-127: LGTM! Correct implementation of the new userEvent API.
The changes properly implement the new async pattern introduced in @testing-library/user-event v14:
- Creates user instance with
userEvent.setup() - Properly awaits all user interactions
- Maintains consistent pattern across all test cases
Also applies to: 134-134, 163-165, 172-172, 241-242, 248-251, 253-260, 272-273, 289-289
src/screens/OrganizationEvents/OrganizationEvents.test.tsx (1)
241-242: LGTM! Consistent implementation of the new userEvent API.
The changes correctly implement the async pattern required by @testing-library/user-event v14 across all test cases:
- Properly initializes user instance with
setup() - Correctly awaits all user interactions
- Maintains consistent implementation across the entire test suite
Also applies to: 248-251, 253-260, 272-273, 339-340, 346-349, 350-357, 369-372, 386-386, 391-391, 423-424, 430-433, 435-443, 455-455, 472-472
.github/workflows/pull-request.yml (2)
Line range hint 301-422: LGTM! Well-structured app startup verification jobs.
The new jobs Start-App-Without-Docker and Docker-Start-Check are well-implemented:
- Proper health checks with timeouts
- Cleanup in
alwaysblocks - Comprehensive error logging
267-271: Verify the coverage threshold change.
The minimum coverage threshold has been significantly reduced from 95.0% to 0.0%. This change could potentially allow uncaught regressions to slip through.
src/screens/OrgList/OrgList.test.tsx (1)
238-283: Well-structured migration to userEvent.setup()
The changes correctly implement the new userEvent.setup() pattern with proper async/await handling for user interactions. This is the recommended approach in @testing-library/user-event v14+.
src/components/RecurrenceOptions/RecurrenceOptions.test.tsx (1)
Line range hint 247-341: Excellent implementation of userEvent.setup()
The changes demonstrate a thorough and consistent implementation of the new userEvent.setup() pattern. All user interactions are properly awaited, and the test structure is well-organized.
src/components/UserPortal/PostCard/PostCard.test.tsx (3)
282-283: LGTM! Correctly migrated to userEvent.setup()
The change properly implements the new userEvent v14 pattern by using setup() and awaiting the click event.
332-340: LGTM! Comprehensive migration of edit post test
The test properly implements multiple userEvent operations with the new v14 API, correctly awaiting all async operations.
389-391: LGTM! Delete post test properly migrated
The test correctly implements the new userEvent v14 pattern for the delete operation.
src/screens/LoginPage/LoginPage.test.tsx (1)
631-632: LGTM! Correct migration to @testing-library/user-event v14
The changes correctly implement the new userEvent patterns by:
- Using
userEvent.setup()to create a user instance - Making all user interactions async with
await user.click/type - Maintaining consistent patterns across all test cases
Also applies to: 635-635, 663-664, 667-667, 695-696, 699-699, 740-741, 743-743, 770-771, 773-773, 800-801, 805-805, 830-831, 837-837, 861-862
src/components/EventListCard/EventListCard.test.tsx (1)
117-118: LGTM! Correct migration to @testing-library/user-event v14
The changes correctly implement the new userEvent patterns by:
- Using
userEvent.setup()to create a user instance - Making all user interactions async with
await user.click/type - Maintaining consistent patterns across all test cases
Also applies to: 123-123, 179-180, 193-193, 206-207, 217-217, 230-231, 239-239, 256-257, 266-266, 283-284, 293-293, 305-306, 312-312, 324-325, 331-331, 342-343, 347-347, 351-351, 355-355, 367-370, 386-387, 391-391, 395-395, 399-399, 421-422, 424-425, 427-427, 440-441, 445-445, 449-449, 453-453, 465-469, 472-472
src/screens/OrganizationPeople/OrganizationPeople.test.tsx (1)
733-733: LGTM! Correct migration to @testing-library/user-event v14
The changes correctly implement the new userEvent patterns by:
- Using
userEvent.setup()to create a user instance - Making all user interactions async with
await user.click/type - Maintaining consistent patterns across all test cases
Also applies to: 735-736, 739-740, 747-747, 938-939, 945-945, 954-954, 982-983, 987-987, 1022-1023, 1027-1027, 1032-1032, 1060-1061, 1065-1065, 1088-1088, 1094-1094, 1099-1099, 1127-1128, 1132-1132, 1155-1155, 1161-1161, 1164-1164, 1192-1193, 1197-1197, 1215-1215, 1221-1221, 1226-1226, 1256-1256, 1258-1259, 1261-1261, 1263-1264, 1267-1267, 1296-1296, 1298-1298, 1300-1300, 1325-1326, 1328-1328, 1331-1331, 1349-1350, 1367-1368, 1369-1369, 1371-1373, 1376-1376, 1425-1426, 1430-1430
src/components/AddOn/core/AddOnStore/AddOnStore.test.tsx (2)
65-66: Great type safety improvement!
Changing from any to unknown is a good practice as it enforces proper type checking before using the values.
321-322: Correctly implemented userEvent.setup()
The change from direct userEvent calls to using setup() is required for @testing-library/user-event v14. This new pattern provides better handling of async user interactions.
src/components/LeftDrawerOrg/LeftDrawerOrg.test.tsx (1)
370-371: Correctly implemented userEvent.setup()
The change follows the recommended pattern for @testing-library/user-event v14, ensuring proper handling of async user interactions.
src/screens/UserPortal/Events/Events.test.tsx (1)
305-306: Well-structured user event implementation
The changes demonstrate excellent implementation of userEvent v14 patterns with consistent async/await usage and proper setup.
Also applies to: 313-314, 318-319, 323-324, 326-327, 329-330, 332-333, 335-335, 337-337
src/screens/BlockUser/BlockUser.test.tsx (1)
375-377: LGTM! The userEvent setup changes are consistent and well-implemented.
The changes correctly implement the new userEvent.setup() pattern from @testing-library/user-event v14, ensuring proper handling of async user interactions.
Also applies to: 383-383, 406-408, 415-415, 438-440, 448-452, 477-479, 487-491, 536-539, 566-568, 591-593
src/components/RecurrenceOptions/CustomRecurrence.test.tsx (1)
213-215: LGTM! Comprehensive update of user event handling.
The changes properly implement the new userEvent.setup() pattern while maintaining the existing test coverage and scenarios.
Also applies to: 223-223, 229-229, 235-235, 245-246, 249-250, 253-253, 284-286, 304-304, 310-310, 315-315, 322-322, 327-327, 341-341, 348-348, 358-358, 365-365, 375-375, 382-382, 415-417, 425-425, 431-431, 437-437, 443-446, 448-448, 479-481, 487-490, 492-494, 497-499, 512-512, 518-518, 524-524, 526-526, 531-531, 539-539, 546-546, 554-554, 580-580, 591-593, 628-630, 636-639, 641-644, 646-649, 662-662, 668-668, 674-674, 676-676, 681-681, 693-693, 714-714, 725-725
src/screens/OrganizationActionItems/ItemModal.test.tsx (1)
761-761: LGTM! Correct usage of fireEvent.submit for form submissions.
The change from fireEvent.click to fireEvent.submit is more semantically accurate for form submission events.
Also applies to: 773-773, 792-792
src/components/OrgPostCard/OrgPostCard.test.tsx (1)
138-147: LGTM! Consistent implementation of userEvent.setup() pattern
The migration from direct userEvent usage to the new setup pattern is implemented correctly and consistently across all test cases. The changes properly handle async interactions as required by @testing-library/user-event v14.
Also applies to: 165-174, 192-195, 212-213, 227-231, 306-310, 376-379, 409-412, 427-431, 464-468, 482-484, 500-503, 521-541, 545-559, 572-586, 608-612, 643-647, 683-686, 709-712, 736-741, 755-758
| const user = userEvent.setup(); | ||
| await user.click(screen.getByText(/Save Changes/i)); |
There was a problem hiding this comment.
Inconsistent userEvent patterns detected
While these changes correctly implement the new async userEvent pattern, the first test case still uses the old synchronous pattern. This inconsistency could lead to flaky tests.
Apply this pattern to the first test case as well:
- userEvent.type(
- screen.getByPlaceholderText(/Previous Password/i),
- formData.previousPassword,
- );
- userEvent.type(
- screen.getAllByPlaceholderText(/New Password/i)[0],
- formData.newPassword,
- );
- userEvent.type(
- screen.getByPlaceholderText(/Confirm New Password/i),
- formData.confirmNewPassword,
- );
- userEvent.click(screen.getByText(/Save Changes/i));
+ const user = userEvent.setup();
+ await user.type(
+ screen.getByPlaceholderText(/Previous Password/i),
+ formData.previousPassword,
+ );
+ await user.type(
+ screen.getAllByPlaceholderText(/New Password/i)[0],
+ formData.newPassword,
+ );
+ await user.type(
+ screen.getByPlaceholderText(/Confirm New Password/i),
+ formData.confirmNewPassword,
+ );
+ await user.click(screen.getByText(/Save Changes/i));Also applies to: 122-136
| userEvent.click(screen.getByTestId(/nextPage/i)); | ||
| userEvent.click(screen.getByTestId(/previousPage/i)); |
There was a problem hiding this comment.
Update userEvent implementation for v14
The implementation needs to be updated to match the new @testing-library/user-event v14 API. The current synchronous usage of userEvent.click could lead to race conditions.
Apply this pattern to both test cases:
- userEvent.click(screen.getByTestId(/nextPage/i));
- userEvent.click(screen.getByTestId(/previousPage/i));
+ const user = userEvent.setup();
+ await user.click(screen.getByTestId(/nextPage/i));
+ await user.click(screen.getByTestId(/previousPage/i));Also applies to: 59-60
| await waitFor(() => { | ||
| expect(global.window.location.pathname).toContain('/users'); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Modernize user event handling throughout the test suite
While the addition of waitFor is good, there are several improvements needed to fully align with @testing-library/user-event v14:
- userEvent now requires setup() before use
- The test mixes direct .click() calls with userEvent.click()
- Some userEvent interactions aren't properly awaited
Here's how to update the test:
describe('Testing Left Drawer component for SUPERADMIN', () => {
test('Component should be rendered properly', async () => {
+ const user = userEvent.setup()
setItem('UserImage', '');
setItem('SuperAdmin', true);
setItem('FirstName', 'John');
setItem('LastName', 'Doe');
// ... render code ...
const orgsBtn = screen.getByTestId(/orgsBtn/i);
const rolesBtn = screen.getByTestId(/rolesBtn/i);
const communityProfileBtn = screen.getByTestId(/communityProfileBtn/i);
await act(async () => {
- orgsBtn.click();
+ await user.click(orgsBtn);
});
expect(
orgsBtn.className.includes('text-black btn btn-success'),
).toBeTruthy();
expect(rolesBtn.className.includes('text-secondary btn')).toBeTruthy();
expect(
communityProfileBtn.className.includes('text-secondary btn'),
).toBeTruthy();
await act(async () => {
- userEvent.click(rolesBtn);
+ await user.click(rolesBtn);
});
await waitFor(() => {
expect(global.window.location.pathname).toContain('/users');
});
await act(async () => {
- userEvent.click(communityProfileBtn);
+ await user.click(communityProfileBtn);
});
});Similar updates should be applied to other test cases in this file.
Committable suggestion skipped: line range outside the PR's diff.
| .chatMessageBelongsTo?._id == props.selectedContact | ||
| ) { | ||
| chatRefetch(); |
There was a problem hiding this comment.
Replace loose equality (==) with strict equality (===)
The loose equality operator (==) can lead to unexpected behavior due to type coercion. Use strict equality (===) for more predictable comparisons.
Apply this change:
- .chatMessageBelongsTo?._id == props.selectedContact
+ .chatMessageBelongsTo?._id === props.selectedContact📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .chatMessageBelongsTo?._id == props.selectedContact | |
| ) { | |
| chatRefetch(); | |
| .chatMessageBelongsTo?._id === props.selectedContact | |
| ) { | |
| chatRefetch(); |
| messageSubscriptionData?.data.data.messageSentToChat | ||
| .chatMessageBelongsTo['_id'] == props.selectedContact | ||
| .chatMessageBelongsTo?._id == props.selectedContact | ||
| ) { | ||
| chatRefetch(); | ||
| } else { | ||
| chatRefetch({ | ||
| id: messageSubscriptionData?.data.data.messageSentToChat | ||
| .chatMessageBelongsTo['_id'], | ||
| .chatMessageBelongsTo?._id, | ||
| }); | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Based on the search results, I can now generate the final response as the code analysis provides sufficient evidence about the subscription handler's implementation and its potential issues.
Add proper null-safety checks to prevent runtime errors
The current implementation has incomplete null checking that could lead to runtime errors:
- While
messageSubscriptionData?.datauses optional chaining, subsequent property accesses like.data.messageSentToChatdon't - The else branch assumes
messageSentToChatandchatMessageBelongsToexist when callingchatRefetch - No early return when critical data is missing
Suggested implementation:
onData: (messageSubscriptionData) => {
const chatId = messageSubscriptionData?.data?.data?.messageSentToChat
?.chatMessageBelongsTo?._id;
if (!chatId) return;
if (chatId === props.selectedContact) {
chatRefetch();
} else {
chatRefetch({ id: chatId });
}
}🔗 Analysis chain
Verify subscription handler's robustness
The subscription handler's logic needs verification to ensure it properly handles all edge cases:
- When messageSubscriptionData is undefined
- When data.data is undefined
- When messageSentToChat is undefined
Let's verify the usage pattern across the codebase:
Consider adding null checks:
onData: (messageSubscriptionData) => {
+ const chatId = messageSubscriptionData?.data?.data?.messageSentToChat
+ ?.chatMessageBelongsTo?._id;
+ if (!chatId) return;
+
if (
- messageSubscriptionData?.data.data.messageSentToChat &&
- messageSubscriptionData?.data.data.messageSentToChat
- .chatMessageBelongsTo?._id === props.selectedContact
+ chatId === props.selectedContact
) {
chatRefetch();
} else {
- chatRefetch({
- id: messageSubscriptionData?.data.data.messageSentToChat
- .chatMessageBelongsTo?._id,
- });
+ chatRefetch({ id: chatId });
}
},🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar subscription handler patterns to verify consistency
ast-grep --pattern 'useSubscription($_, {
$$$
onData: ($_) => {
$$$
},
$$$
})'
# Search for potential null checks before accessing nested properties
rg -A 3 'messageSubscriptionData\?\.data'
Length of output: 2445
| const user = userEvent.setup(); | ||
| await user.click(screen.getByTestId('createPostModalBtn')); | ||
|
|
||
| userEvent.type(screen.getByTestId('modalTitle'), formData.posttitle); | ||
| await user.type(screen.getByTestId('modalTitle'), formData.posttitle); | ||
|
|
||
| userEvent.type(screen.getByTestId('modalinfo'), formData.postinfo); | ||
| userEvent.upload(screen.getByTestId('addMediaField'), formData.postImage); | ||
| userEvent.upload(screen.getByTestId('addMediaField'), formData.postVideo); | ||
| userEvent.upload(screen.getByTestId('addMediaField'), formData.postImage); | ||
| userEvent.upload(screen.getByTestId('addMediaField'), formData.postVideo); | ||
| userEvent.click(screen.getByTestId('pinPost')); | ||
| await user.type(screen.getByTestId('modalinfo'), formData.postinfo); | ||
| await user.upload(screen.getByTestId('addMediaField'), formData.postImage); | ||
| await user.upload(screen.getByTestId('addMediaField'), formData.postVideo); | ||
| await user.upload(screen.getByTestId('addMediaField'), formData.postImage); | ||
| await user.upload(screen.getByTestId('addMediaField'), formData.postVideo); | ||
| await user.click(screen.getByTestId('pinPost')); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Standardize event handling approach
While some parts of the test have been migrated to userEvent v14, there's still inconsistent usage of fireEvent. For better maintainability and consistent behavior, consider migrating all user interactions to userEvent.
Example migration for one test case:
const user = userEvent.setup();
await user.click(screen.getByTestId('createPostModalBtn'));
- const postTitleInput = screen.getByTestId('modalTitle');
- fireEvent.change(postTitleInput, { target: { value: 'Test Post' } });
+ await user.type(screen.getByTestId('modalTitle'), 'Test Post');
- const postInfoTextarea = screen.getByTestId('modalinfo');
- fireEvent.change(postInfoTextarea, {
- target: { value: 'Test post information' },
- });
+ await user.type(screen.getByTestId('modalinfo'), 'Test post information');Also applies to: 397-409
| await waitFor(() => { | ||
| expect(toast.success).toHaveBeenCalledWith(translations.eventUpdated); | ||
| console.log(translations.eventUpdated); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace console.log with proper test assertions
Several test cases use console.log instead of proper test assertions. Consider replacing these with expect statements to properly verify the test outcomes.
Example:
- console.log(translations.eventUpdated);
+ expect(toast.success).toHaveBeenCalledWith(translations.eventUpdated);Also applies to: 472-472, 712-712, 779-779, 842-842, 883-883
| dropdownToggles.forEach(async (dropdownToggle) => { | ||
| await user.click(dropdownToggle); |
There was a problem hiding this comment.
Fix async forEach loops
The forEach loops with async callbacks may not work as expected since forEach doesn't wait for async operations. Consider using a for...of loop or Promise.all with map instead.
Example:
- dropdownToggles.forEach(async (dropdownToggle) => {
- await user.click(dropdownToggle);
- });
+ for (const dropdownToggle of dropdownToggles) {
+ await user.click(dropdownToggle);
+ }Or:
- dropdownToggles.forEach(async (dropdownToggle) => {
- await user.click(dropdownToggle);
- });
+ await Promise.all(
+ dropdownToggles.map((dropdownToggle) => user.click(dropdownToggle))
+ );Also applies to: 1258-1259, 1368-1369
| const user = userEvent.setup(); | ||
| await user.click(startPostBtn); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update user event patterns for better async handling
The changes correctly implement the new userEvent v14 patterns with proper async handling. However, some interactions are using a mix of userEvent and fireEvent methods.
Consider consistently using userEvent for all interactions as it better simulates real user behavior:
- fireEvent.click(startPostBtn);
+ await user.click(startPostBtn);
- fireEvent.change(screen.getByTestId('postInput'), {
- target: { value: 'some content' },
- });
+ await user.type(screen.getByTestId('postInput'), 'some content');Also applies to: 313-314, 318-319, 323-324, 326-327, 329-330, 332-333, 335-335, 337-337
| await act(async () => { | ||
| renderManageTag(link); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify async test patterns
The code wraps render calls in unnecessary act() blocks and uses inconsistent patterns for async operations.
Consider these improvements:
- Remove unnecessary act() wrappers around render:
- await act(async () => {
- renderManageTag(link);
- });
+ renderManageTag(link);- Standardize async waiting patterns:
- await waitFor(() => {
- expect(screen.getByTestId('addPeopleToTagBtn')).toBeInTheDocument();
- });
+ expect(await screen.findByTestId('addPeopleToTagBtn')).toBeInTheDocument();Also applies to: 148-150, 172-174, 199-201, 226-228, 250-252, 274-276, 291-293, 308-310, 327-329, 344-346, 367-369, 457-459, 478-480, 510-512
mohamedrehan1
force-pushed
the
develop-postgres
branch
from
7fa32de to
753e5b4
Compare
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (26)
src/components/Venues/VenueModal.tsx (1)
Line range hint
263-269: Consider adding aria-label for better accessibilityWhile the button styling changes look good, consider adding an aria-label to improve accessibility for screen readers.
<Button type="submit" className={styles.addButton} value={edit ? 'editVenue' : 'createVenue'} data-testid={edit ? 'updateVenueBtn' : 'createVenueBtn'} + aria-label={edit ? t('editVenue') : t('createVenue')} onClick={handleSubmit} disabled={loading} >src/components/AddOn/support/components/Action/Action.spec.tsx (1)
1-13: Consider enhancing test setup with userEventSince this PR upgrades
@testing-library/user-event, consider importing and using it for any interaction testing that might be needed. Also, the JSDoc could be more specific about what the Action component does./** * Unit tests for the Action component. * - * This file contains tests for the Action component to ensure it behaves as expected - * under various scenarios. + * The Action component [brief description of what it does]. + * These tests verify: + * - Proper rendering of label and children + * - [other key behaviors to test] */ import React from 'react'; -import { render } from '@testing-library/react'; +import { render, screen } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; import { Provider } from 'react-redux'; import { describe, test, expect } from 'vitest';src/components/Advertisements/core/AdvertisementRegister/AdvertisementRegister.tsx (3)
3-3: Consider using path aliases for style importsThe deep relative path (
../../../../) makes the code fragile and harder to maintain. Consider using path aliases (e.g.,@styles/app.module.css) to improve maintainability and prevent path-related issues during file moves.
Line range hint
408-418: Enhance button accessibilityWhile the styling has been updated, consider adding
aria-labelattributes to improve accessibility:<Button variant="secondary" onClick={handleClose} className={styles.closeButton} data-testid="addonclose" + aria-label={tCommon('close')} > {tCommon('close')} </Button> {formStatus === 'register' ? ( <Button variant="primary" onClick={handleRegister} data-testid="addonregister" className={styles.addButton} + aria-label={tCommon('register')} > {tCommon('register')} </Button>
Line range hint
297-309: Add file upload validationsThe current file upload implementation lacks important security validations:
- File size limits to prevent memory issues
- Strict file type validation beyond just MIME type checks
Consider adding these validations:
onChange={async ( e: React.ChangeEvent<HTMLInputElement>, ): Promise<void> => { const target = e.target as HTMLInputElement; const file = target.files && target.files[0]; if (file) { + // Validate file size (e.g., 5MB limit) + const MAX_FILE_SIZE = 5 * 1024 * 1024; + if (file.size > MAX_FILE_SIZE) { + toast.error(t('fileTooLarge') as string); + return; + } + + // Validate file type + const allowedTypes = ['image/jpeg', 'image/png', 'image/gif', 'video/mp4']; + if (!allowedTypes.includes(file.type)) { + toast.error(t('invalidFileType') as string); + return; + } + const mediaBase64 = await convertToBase64(file); setFormState({ ...formState, advertisementMedia: mediaBase64, }); } }}src/components/AddOn/core/AddOnEntry/AddOnEntry.spec.tsx (2)
48-53: Improve test isolation by avoiding global mock stateUsing a global
mockIDvariable can lead to test interdependence and hard-to-debug issues. Consider using a more isolated approach.-let mockID: string | undefined = '1'; - -vi.mock('react-router-dom', async () => ({ - ...(await vi.importActual('react-router-dom')), - useParams: () => ({ orgId: mockID }), -})); +// In each test that needs different orgId: +beforeEach(() => { + vi.mock('react-router-dom', async () => ({ + ...(await vi.importActual('react-router-dom')), + useParams: () => ({ orgId: 'test-specific-id' }), + })); +}); + +afterEach(() => { + vi.resetModules(); +});
56-70: Reduce test props duplication using a shared fixtureThe test props are duplicated across multiple tests. Consider extracting them into a shared fixture.
+const createTestProps = (overrides = {}) => ({ + id: '1', + title: 'Test Addon', + description: 'Test addon description', + createdBy: 'Test User', + component: 'string', + installed: true, + configurable: true, + modified: true, + isInstalled: true, + uninstalledOrgs: [], + enabled: true, + getInstalledPlugins: () => ({ sample: 'sample' }), + ...overrides +}); // In tests: -const props = { ... } +const props = createTestProps(); // Or with overrides: +const props = createTestProps({ uninstalledOrgs: ['undefined'] });Also applies to: 113-128, 148-163, 193-208
vitest.config.ts (1)
9-11: Verify the necessity of 'nodePolyfills' for 'events'.Please ensure that including the
eventsmodule via thenodePolyfillsplugin is necessary for your project. If theeventsmodule is not used in a browser environment, you might consider removing this plugin to reduce bundle size and improve performance.src/style/app.module.css (3)
76-79: Remove redundantmargindeclaration in.btnsContainerclassThe
margin: 2.5rem 0;declaration is repeated in lines 73 and 79 within the.btnsContainerclass. This redundancy can be eliminated to keep the code clean and maintainable.Apply this diff to remove the redundant
margindeclaration:.btnsContainer { display: flex; margin: 2.5rem 0; + align-items: center; + gap: 10px; + /* Adjust spacing between items */ - margin: 2.5rem 0; }
110-143: Simplify class names for improved readabilityThe class names like
.btnsContainerBlockAndUnblockand.btnsBlockBlockAndUnblockare quite lengthy, which may impact readability and maintainability. Consider using shorter, more descriptive class names to enhance clarity.
216-233: Consolidate similar styles to reduce redundancyThe
.searchButton:hoverand.searchclasses share similar styling properties. Combining these styles or utilizing a shared class can reduce code duplication and improve maintainability.src/components/LeftDrawer/LeftDrawer.tsx (1)
66-70: Consider moving inline styles to CSS moduleThe inline styles for the active state could be moved to the CSS module for better maintainability and consistency.
-style={{ - backgroundColor: isActive === true ? '#EAEBEF' : '', -}}Add to
LeftDrawer.module.css:.activeButton { background-color: #EAEBEF; }Then use it in the component:
+className={`${ + isActive === true ? 'text-black ' + styles.activeButton : 'text-secondary' +}`}src/components/EventCalendar/EventCalendar.module.css (1)
287-298: Consider consolidating duplicate hover styles.The
.createButtonand.createButton:hoverstyles have duplicate properties that could be consolidated.Consider this refactor:
.createButton { background-color: var(--grey-bg-color) !important; color: black !important; border: 1px solid var(--dropdown-border-color); } .createButton:hover { - background-color: var(--grey-bg-color) !important; - color: black !important; border: 1px solid var(--dropdown-border-color) !important; }src/screens/OrganizationVenues/OrganizationVenues.tsx (1)
149-161: Consider adding aria-label for accessibility.The search input and button could benefit from improved accessibility.
Apply these changes:
<Form.Control type="name" id="searchByName" className={styles.inputField} placeholder={t('searchBy') + ' ' + tCommon(searchBy)} data-testid="searchBy" autoComplete="off" required value={searchTerm} onChange={handleSearchChange} + aria-label={t('searchBy')} /> <Button className={styles.searchButton} data-testid="searchBtn" + aria-label={t('search')} >src/screens/Requests/Requests.tsx (1)
261-261: Remove commented style codeThe inline style comment can be removed as it's now handled by the CSS module.
- // style={{ marginTop: '9px' }}src/screens/OrganizationFunds/OrganizationFunds.tsx (2)
248-249: Remove commented button classThe commented class can be removed as it's been replaced by the CSS module.
- // className="me-2 rounded"
341-343: Consider moving inline style to CSS moduleFor consistency with other style changes, consider moving the marginTop style to the CSS module.
- className={styles.createButton} - style={{ marginTop: '11px' }} + className={`${styles.createButton} ${styles.createButtonSpacing}`}src/screens/OrganizationActionItems/OrganizationActionItems.tsx (1)
Line range hint
12-24: Review discount and fee structure logicThe current implementation could lead to unintended consequences where customers with lower discount tiers (e.g., 10% for 3-4 years loyalty) end up paying more than without a discount on small purchases due to the flat $20 fee.
Example:
- Purchase: $100
- 10% discount: -$10
- Flat fee: +$20
- Final price: $110 (more than original price)
Consider one of these alternatives:
- Scale the fee based on purchase amount
- Apply fee before discount
- Set minimum purchase amount for fee
- Remove fee for loyal customers
Would you like assistance implementing any of these alternatives?
src/screens/LoginPage/LoginPage.tsx (2)
Line range hint
591-601: Enhance email validationThe current email validation only checks for length >= 8. Consider adding a proper email format validation using a regex pattern or the HTML5 email input validation.
if ( isValidName(signfirstName) && isValidName(signlastName) && signfirstName.trim().length > 1 && signlastName.trim().length > 1 && - signEmail.length >= 8 && + /^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(signEmail) && signPassword.length > 1 && validatePassword(signPassword) ) {
Line range hint
1-853: Consider breaking down the component for better maintainabilityThe component is quite large and handles multiple responsibilities (login, registration, password validation, etc.). Consider:
- Splitting into smaller components (LoginForm, RegistrationForm, PasswordValidation)
- Using a form management library like Formik or React Hook Form
- Centralizing error handling logic
This would improve:
- Code maintainability
- Testing capabilities
- Reusability
src/assets/css/app.css (4)
143-144: Consider using relative units for modal dimensionsWhile the modal width variables are good for consistency, consider using relative units (e.g., vw) with max-width constraints to ensure better responsiveness on different screen sizes.
- --modal-width: 670px; - --modal-max-width: 680px; + --modal-width: min(670px, 90vw); + --modal-max-width: min(680px, 95vw);
151-154: Improve fallback values for table stylingThe table styling variables that reference other variables should have explicit fallback values to prevent potential styling issues if the referenced variables are undefined.
- --table-head-bg: var(--bs-primary, blue); - --table-header-color: var(--bs-greyish-black, black); + --table-head-bg: var(--bs-primary, #0d6efd); + --table-header-color: var(--bs-greyish-black, #212529);
146-149: Consider adding hover states for button colorsThe button-related color variables should include hover states to maintain consistent interaction behavior.
--delete-button-bg: #f8d6dc; --delete-button-color: #ff4d4f; + --delete-button-hover-bg: #ffcdd2; + --delete-button-hover-color: #f44336; --search-button-bg: #a8c7fa; --search-button-border: #555555; + --search-button-hover-bg: #90caf9;
156-158: Ensure consistent background color variablesThe table and row background colors should be consistent with the design system's color palette. Consider using the existing Bootstrap background variables as a base.
- --table-bg-color: #eaebef; - --tablerow-bg-color: #eff1f7; - --row-background: var(--bs-white, white); + --table-bg-color: var(--bs-gray-100, #eaebef); + --tablerow-bg-color: var(--bs-gray-50, #eff1f7); + --row-background: var(--bs-white, #ffffff);src/screens/OrganizationEvents/OrganizationEvents.module.css (2)
64-64: Consider using CSS variables for consistent border colors.The border color
#eaebefis used consistently across multiple title elements. Consider defining it as a CSS variable for better maintainability.:root { + --title-border-color: #eaebef; } .logintitle { - border-bottom: 3px solid #eaebef; + border-bottom: 3px solid var(--title-border-color); }Also applies to: 75-75, 155-155, 165-165
200-200: Consider using CSS variables for button colors.For consistency with other color usage in the file, consider using a CSS variable for the button background color.
:root { + --button-bg-color: #eaebef; } .greenregbtn { - background-color: #eaebef; + background-color: var(--button-bg-color); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (34)
.eslintignore(1 hunks).eslintrc.json(3 hunks).github/workflows/pull-request.yml(8 hunks)package.json(6 hunks)src/assets/css/app.css(1 hunks)src/components/AddOn/core/AddOnEntry/AddOnEntry.spec.tsx(1 hunks)src/components/AddOn/support/components/Action/Action.spec.tsx(1 hunks)src/components/Advertisements/core/AdvertisementRegister/AdvertisementRegister.tsx(4 hunks)src/components/CheckIn/tagTemplate.ts(1 hunks)src/components/EventCalendar/EventCalendar.module.css(15 hunks)src/components/EventCalendar/EventHeader.tsx(2 hunks)src/components/LeftDrawer/LeftDrawer.test.tsx(5 hunks)src/components/LeftDrawer/LeftDrawer.tsx(1 hunks)src/components/LoginPortalToggle/LoginPortalToggle.module.css(1 hunks)src/components/OrgListCard/OrgListCard.module.css(1 hunks)src/components/Venues/VenueModal.tsx(3 hunks)src/screens/BlockUser/BlockUser.tsx(8 hunks)src/screens/LoginPage/LoginPage.module.css(1 hunks)src/screens/LoginPage/LoginPage.tsx(4 hunks)src/screens/OrgList/OrgList.module.css(2 hunks)src/screens/OrgList/OrgList.tsx(6 hunks)src/screens/OrgPost/OrgPost.tsx(9 hunks)src/screens/OrganizationActionItems/OrganizationActionItems.tsx(2 hunks)src/screens/OrganizationEvents/OrganizationEvents.module.css(9 hunks)src/screens/OrganizationEvents/OrganizationEvents.tsx(2 hunks)src/screens/OrganizationFunds/FundModal.tsx(3 hunks)src/screens/OrganizationFunds/OrganizationFunds.tsx(5 hunks)src/screens/OrganizationTags/OrganizationTags.tsx(1 hunks)src/screens/OrganizationVenues/OrganizationVenues.tsx(4 hunks)src/screens/Requests/Requests.module.css(0 hunks)src/screens/Requests/Requests.tsx(4 hunks)src/style/app.module.css(10 hunks)vitest.config.ts(1 hunks)vitest.setup.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/screens/Requests/Requests.module.css
✅ Files skipped from review due to trivial changes (4)
- src/screens/OrganizationTags/OrganizationTags.tsx
- vitest.setup.ts
- src/components/OrgListCard/OrgListCard.module.css
- src/components/CheckIn/tagTemplate.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- .eslintignore
- package.json
🧰 Additional context used
📓 Learnings (2)
src/screens/OrganizationFunds/FundModal.tsx (1)
Learnt from: GlenDsza
PR: PalisadoesFoundation/talawa-admin#2064
File: src/screens/OrganizationFunds/OrganizationFunds.tsx:66-72
Timestamp: 2024-11-12T10:40:58.654Z
Learning: Renaming the Enum `Modal` to `ModalState` in the `organizationFunds` component is unnecessary and does not change the functionality.
src/components/AddOn/support/components/Action/Action.spec.tsx (1)
Learnt from: adithyanotfound
PR: PalisadoesFoundation/talawa-admin#2482
File: src/components/AddOn/support/components/Action/Action.spec.tsx:1-8
Timestamp: 2024-11-27T06:37:47.871Z
Learning: In the Talawa-Admin project, the `@testing-library/jest-dom` package is imported globally in `vitest.setup.ts`, so individual test files do not need to import it separately.
🪛 Biome (1.9.4)
src/components/EventCalendar/EventCalendar.module.css
[error] 14-14: Unexpected shorthand property margin after margin-bottom
(lint/suspicious/noShorthandPropertyOverrides)
🪛 yamllint (1.35.1)
.github/workflows/pull-request.yml
[error] 49-49: trailing spaces
(trailing-spaces)
[error] 60-60: trailing spaces
(trailing-spaces)
[error] 63-63: trailing spaces
(trailing-spaces)
[error] 92-92: trailing spaces
(trailing-spaces)
[error] 133-133: trailing spaces
(trailing-spaces)
[error] 136-136: trailing spaces
(trailing-spaces)
[error] 157-157: trailing spaces
(trailing-spaces)
[error] 164-164: trailing spaces
(trailing-spaces)
[error] 204-204: trailing spaces
(trailing-spaces)
[error] 208-208: trailing spaces
(trailing-spaces)
[error] 230-230: trailing spaces
(trailing-spaces)
[error] 231-231: trailing spaces
(trailing-spaces)
[error] 270-270: trailing spaces
(trailing-spaces)
[error] 276-276: trailing spaces
(trailing-spaces)
[error] 278-278: trailing spaces
(trailing-spaces)
🪛 actionlint (1.7.4)
.github/workflows/pull-request.yml
62-62: shellcheck reported issue in this script: SC2086:info:1:12: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (48)
src/components/Venues/VenueModal.tsx (1)
Line range hint 89-101: LGTM: Robust form validation implementation
The validation logic is well-implemented with:
- Proper empty/whitespace check for venue name
- Numeric validation for capacity ensuring positive values
- Internationalized error messages
src/components/AddOn/support/components/Action/Action.spec.tsx (1)
15-31: 🛠️ Refactor suggestion
Enhance test coverage with additional test cases
The current test only verifies basic rendering. Consider adding:
- Interaction tests using the new userEvent
- Edge cases and error scenarios
- TypeScript types for props
Here's a suggested enhancement:
interface ActionProps {
children: React.ReactNode;
label: string;
}
describe('Testing Action Component', () => {
const mockProps: ActionProps = {
children: 'dummy children',
label: 'dummy label',
};
const setup = (props = mockProps) => {
const user = userEvent.setup();
return {
user,
...render(
<Provider store={store}>
<Action {...props} />
</Provider>
),
};
};
test('renders props correctly', () => {
setup();
expect(screen.getByText(mockProps.label)).toBeInTheDocument();
expect(screen.getByText(mockProps.children)).toBeInTheDocument();
});
test('handles empty label', () => {
setup({ ...mockProps, label: '' });
expect(screen.getByText(mockProps.children)).toBeInTheDocument();
});
// Add more test cases for:
// - User interactions
// - Error scenarios
// - Edge cases
});Let's check if the Action component has any event handlers that need testing:
src/components/Advertisements/core/AdvertisementRegister/AdvertisementRegister.tsx (1)
266-266: Verify modal header styling after class removal
Please ensure that the modal header styling remains consistent after removing the class name. Consider documenting any intentional style changes.
✅ Verification successful
Modal header styling is consistent with global patterns
The removal of the class name from Modal.Header is consistent with the codebase patterns. The search results show that most Modal.Header components either:
- Use no custom className (like in AddOnRegister, OrgPost)
- Use specific styling for special cases (like bg-primary for colored headers)
The file imports global styles from app.module.css, which ensures consistent styling across modals.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any other modal headers in the codebase to ensure consistent styling
rg -l "Modal.Header" | xargs rg "className.*Modal.Header"
Length of output: 57
Script:
#!/bin/bash
# Let's first check for any Modal.Header usage patterns in the codebase
rg "Modal\.Header" -B 2 -A 2
# Also check the specific file for any style-related imports or definitions
rg "import.*style|\.css|\.scss" "src/components/Advertisements/core/AdvertisementRegister/AdvertisementRegister.tsx"
Length of output: 55051
vitest.config.ts (3)
1-1: Import statement correctly updated.
The import of defineConfig from 'vitest/config' is appropriate for Vitest configurations and resolves previous merge conflicts.
14-18: Test configuration is properly set up.
The test configuration includes the necessary settings for Vitest:
includespecifies the correct pattern for test files.globalsis set totrue, enabling global variables in tests.environmentis set to'jsdom', which is suitable for testing React components.setupFilesis correctly pointing to'vitest.setup.ts'.
19-34: Coverage configuration is comprehensive.
The coverage settings are well-defined:
- Coverage is enabled and uses the
'istanbul'provider. - Reports are stored in
'./coverage/vitest'. - The
excludearray appropriately omits files and directories that should not be included in coverage reports. - The
reporterformats include a variety of outputs suitable for different use cases.
src/style/app.module.css (4)
69-69: Change to .input class appears appropriate
Updating flex: 3; in the .input class should adjust the input field's sizing to better fit the layout.
102-109: New .inputField class is well-structured
The addition of the .inputField class enhances the styling of input fields, improving the user interface with consistent margins, background color, and subtle shadowing.
473-496: New .custom_table styling enhances table appearance
The .custom_table class and its associated styles improve the visual presentation of tables, adding rounded corners and hover effects that enhance user interaction.
551-556: .notFound class properly manages empty state display
The .notFound class effectively centers and aligns content when no results are found, ensuring a consistent user experience.
src/components/LoginPortalToggle/LoginPortalToggle.module.css (2)
27-27: Use of CSS variables for theme consistency
Changing the background color to var(--active-button-bg) in the .activeLink class enhances theming capabilities and maintains consistency across the application.
32-32: Ensure --active-hover variable is defined
Please verify that the CSS variable --active-hover is defined. If it's not, this could lead to unexpected styles when hovering over active links.
.eslintrc.json (2)
Line range hint 79-122: Confirm variable naming conventions in ESLint configuration
In the @typescript-eslint/naming-convention rule, variables are allowed to be in PascalCase, camelCase, or UPPER_CASE. Typically, variables use camelCase, and PascalCase is reserved for types and classes. Please confirm if allowing PascalCase for variables is intentional.
146-146: Addition of ignorePatterns is appropriate
Including "ignorePatterns": ["**/*.css", "**/*.scss", "**/*.less", "**/*.json"] ensures that ESLint doesn't process non-JavaScript/TypeScript files, which is a good practice.
src/components/EventCalendar/EventHeader.tsx (2)
58-58: Verify the new styles.searchbutton class includes necessary styles
Replacing the Bootstrap utility classes with styles.searchbutton shifts styling responsibility to the CSS module. Ensure that styles.searchbutton includes all required styles to maintain the button's appearance and functionality.
115-115: Update to styles.createButton maintains styling consistency
Changing the class to styles.createButton should align with the application's styling conventions and improve consistency across components.
src/screens/LoginPage/LoginPage.module.css (3)
75-86: New .email_button class enhances button positioning
The .email_button class is well-defined to position the email button correctly within the layout, ensuring it remains accessible and visually consistent.
88-97: Ensure login_btn colors meet accessibility standards
Please verify that the background color var(--search-button-bg) and text color in .login_btn provide sufficient contrast to meet WCAG accessibility guidelines.
98-108: Ensure reg_btn colors meet accessibility standards
Similarly, confirm that the background and text colors in .reg_btn offer adequate contrast for users, adhering to accessibility requirements.
src/components/LeftDrawer/LeftDrawer.tsx (1)
75-79: LGTM!
The icon fill property changes align well with the new color scheme and properly utilize CSS variables.
src/screens/OrgList/OrgList.module.css (2)
23-24: LGTM!
The color changes for the plugin store button create a more neutral and professional appearance, with consistent hover states.
Also applies to: 30-31
68-68: LGTM!
The new sample organization section styles are well-structured and provide clear visual feedback for user interactions.
src/components/LeftDrawer/LeftDrawer.test.tsx (3)
2-2: LGTM!
The addition of the waitFor import is appropriate for handling asynchronous assertions.
98-98: LGTM!
The class name assertions have been correctly updated to match the component changes.
Also applies to: 184-184, 214-214
109-111: Modernize user event handling
The addition of waitFor is good, but the test still needs to be updated to fully align with @testing-library/user-event v14 best practices.
src/screens/OrganizationVenues/OrganizationVenues.tsx (1)
Line range hint 231-237: LGTM: Clean implementation of create venue button.
The button implementation follows best practices with proper event handling and translations.
src/screens/OrganizationFunds/FundModal.tsx (1)
5-5: Style refactoring looks good!
The changes align with the project-wide effort to centralize styles and standardize button class names. The functionality remains unchanged while improving maintainability.
Also applies to: 176-176, 275-275
src/screens/Requests/Requests.tsx (1)
16-16: Layout structure improvements look good!
The addition of flex utilities and standardized container classes enhances the component's layout structure while maintaining consistency with other components.
Also applies to: 234-237
src/screens/OrganizationFunds/OrganizationFunds.tsx (1)
21-40: Improved DataGrid styling and accessibility!
The updated DataGrid styles enhance visual hierarchy with better hover states and focus indicators. The solid outline on focus improves accessibility.
src/screens/BlockUser/BlockUser.tsx (2)
17-17: LGTM: Style module import updated for consistency.
The change from local to global styles aligns with the standardization of styling across components.
Line range hint 288-288: LGTM: Button styling classes standardized.
Button class names have been updated to use consistent naming conventions across the application:
closeButtonfor modal close buttonscreateButtonfor action buttons
Also applies to: 494-494
.github/workflows/pull-request.yml (2)
209-230: LGTM: Test coverage reporting improved.
Good addition of separate Jest and Vitest test runs with coverage merging. This ensures comprehensive test coverage reporting.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 230-230: trailing spaces
(trailing-spaces)
Line range hint 282-391: LGTM: Comprehensive app startup verification added.
The new Start-App-Without-Docker job provides thorough health checks for both production and development environments.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 270-270: trailing spaces
(trailing-spaces)
[error] 276-276: trailing spaces
(trailing-spaces)
[error] 278-278: trailing spaces
(trailing-spaces)
src/screens/OrganizationEvents/OrganizationEvents.tsx (2)
Line range hint 35-39: LGTM: ViewType enum improves type safety.
Good addition of the ViewType enum for view selection. This provides better type safety and autocompletion.
288-288: LGTM: Button styling classes standardized.
Button class names have been updated to use consistent naming conventions:
closeButtonfor modal close buttonscreateButtonfor action buttons
Also applies to: 494-494
src/screens/OrganizationActionItems/OrganizationActionItems.tsx (1)
369-370: Consider accessibility implications of removing tabIndex
The removal of tabIndex from the search button could affect keyboard navigation. While the button is still focusable by default, verify that the search functionality remains accessible via keyboard.
Also applies to: 386-387
src/screens/OrgList/OrgList.tsx (3)
Line range hint 344-353: LGTM!
The styling changes maintain functionality while improving consistency with the application's style system.
368-369: LGTM!
The dropdown styling changes align with the application's modular CSS approach.
Line range hint 537-554: LGTM!
The modal styling changes maintain functionality while improving consistency with the application's style system.
src/screens/OrgPost/OrgPost.tsx (4)
293-300: LGTM!
The dynamic placeholder text improves user experience by clearly indicating the current search context.
Line range hint 313-349: LGTM!
The dropdown styling changes maintain consistency with the application's modular CSS approach.
375-375: LGTM!
The button styling changes improve layout spacing while maintaining functionality.
554-565: LGTM!
The modal footer button changes improve form handling while maintaining styling consistency.
src/screens/LoginPage/LoginPage.tsx (1)
447-447: LGTM: Improved button styling with CSS modules
The migration from inline styles to CSS modules improves maintainability and follows React best practices.
Also applies to: 474-474, 510-510, 523-523
src/assets/css/app.css (1)
137-142: LGTM: Well-structured dropdown styling variables
The new dropdown-related CSS custom properties are well organized and follow a consistent naming convention. The color values are appropriately defined using hex codes.
src/screens/OrganizationEvents/OrganizationEvents.module.css (3)
Line range hint 35-83: LGTM! Well-structured layout classes.
The new layout classes follow flexbox best practices and maintain a clear, consistent structure.
169-180: LGTM! Good use of CSS variables and hover states.
The close button styles follow best practices by using semantic CSS variables and providing clear hover feedback.
Line range hint 232-305: LGTM! Well-implemented loader and utility classes.
The loader animation and utility classes are well-structured and follow CSS best practices.
src/components/Venues/VenueModal.tsx
Outdated
| @@ -1,6 +1,6 @@ | |||
| import React, { useCallback, useEffect, useRef, useState } from 'react'; | |||
| import { Button, Form, Modal } from 'react-bootstrap'; | |||
| import styles from './VenueModal.module.css'; | |||
| import styles from '../../style/app.module.css'; | |||
There was a problem hiding this comment.
💡 Codebase verification
Style class .titlemodal is missing from global stylesheet
The component uses the following classes from styles:
.titlemodal(missing in global stylesheet).closeButton(present).preview(present).closeButtonP(missing in global stylesheet).addButton(present)
Two required style classes (.titlemodal and .closeButtonP) are not defined in the global stylesheet, which will cause styling issues.
🔗 Analysis chain
Verify style class availability in global stylesheet
The change from local to global styles aligns with the standardization effort, but we should verify the existence of all required style classes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if all required style classes exist in the new global stylesheet
rg -A 1 "\.(?:titlemodal|closeButton|preview|closeButtonP|addButton)\s*{" "src/style/app.module.css"
Length of output: 290
Script:
#!/bin/bash
# Let's fix the regex and check for these style classes
rg "\.titlemodal|\.closeButton|\.preview|\.closeButtonP|\.addButton" "src/style/app.module.css"
# Also let's check what classes are actually used in the component
rg "className=.*?styles\." "src/components/Venues/VenueModal.tsx"
Length of output: 480
| await userEvent.click(btn); | ||
| expect(btn.innerHTML).toMatch(/Install/i); |
There was a problem hiding this comment.
Update userEvent usage for v14 compatibility
The userEvent usage needs to be updated to match v14 API which returns promises.
-await userEvent.click(btn);
-expect(btn.innerHTML).toMatch(/Install/i);
+const user = userEvent.setup();
+await user.click(btn);
+expect(btn.innerHTML).toMatch(/Install/i);
-await userEvent.click(btn);
-expect(btn.innerHTML).toMatch(/Uninstall/i);
+await user.click(btn);
+expect(btn.innerHTML).toMatch(/Uninstall/i);Also applies to: 185-186
| authorization: 'Bearer ' + getItem('token') || '', | ||
| }, | ||
| }); | ||
| console.error = vi.fn(); |
There was a problem hiding this comment.
Avoid global console.error mocking
Mocking console.error globally can hide important errors. Consider moving this into individual tests where needed or use a more targeted approach.
-console.error = vi.fn();
+// Move this into relevant test cases:
+beforeEach(() => {
+ vi.spyOn(console, 'error').mockImplementation(() => {});
+});
+afterEach(() => {
+ vi.restoreAllMocks();
+});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.error = vi.fn(); | |
| beforeEach(() => { | |
| vi.spyOn(console, 'error').mockImplementation(() => {}); | |
| }); | |
| afterEach(() => { | |
| vi.restoreAllMocks(); | |
| }); |
| </Provider> | ||
| </MockedProvider>, | ||
| ); | ||
| await wait(100); |
There was a problem hiding this comment.
Replace arbitrary wait with proper async utilities
Using wait(100) is unreliable and can lead to flaky tests. Use proper async utilities from @testing-library/react.
-await wait(100);
+await screen.findByTestId('AddOnEntry_btn_install');Committable suggestion skipped: line range outside the PR's diff.
| const httpLink = new HttpLink({ | ||
| uri: BACKEND_URL, | ||
| headers: { | ||
| authorization: 'Bearer ' + getItem('token') || '', | ||
| }, | ||
| }); | ||
| console.error = vi.fn(); | ||
| const client: ApolloClient<NormalizedCacheObject> = new ApolloClient({ | ||
| cache: new InMemoryCache(), | ||
| link: ApolloLink.from([httpLink]), | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify Apollo client configuration
The current setup creates confusion by mixing real HTTP client with mock client. The httpLink is never used as it's overridden by StaticMockLink.
-const httpLink = new HttpLink({
- uri: BACKEND_URL,
- headers: {
- authorization: 'Bearer ' + getItem('token') || '',
- },
-});
-const client: ApolloClient<NormalizedCacheObject> = new ApolloClient({
- cache: new InMemoryCache(),
- link: ApolloLink.from([httpLink]),
-});
+const client: ApolloClient<NormalizedCacheObject> = new ApolloClient({
+ cache: new InMemoryCache(),
+ link: link, // Use the StaticMockLink directly
+});Committable suggestion skipped: line range outside the PR's diff.
| display: flex; | ||
| margin-bottom: 2rem; | ||
| align-items: center; | ||
| margin: 0px 10px 0px 10px; | ||
| } |
There was a problem hiding this comment.
Fix conflicting margin properties.
The margin shorthand property overrides the previously set margin-bottom.
Apply this fix:
.calendar__header {
display: flex;
- margin-bottom: 2rem;
align-items: center;
- margin: 0px 10px 0px 10px;
+ margin: 0px 10px 2rem 10px;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| display: flex; | |
| margin-bottom: 2rem; | |
| align-items: center; | |
| margin: 0px 10px 0px 10px; | |
| } | |
| display: flex; | |
| align-items: center; | |
| margin: 0px 10px 2rem 10px; | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 14-14: Unexpected shorthand property margin after margin-bottom
(lint/suspicious/noShorthandPropertyOverrides)
.github/workflows/pull-request.yml
Outdated
| path: "./coverage/lcov.info" | ||
| min_coverage: 0.0 |
There was a problem hiding this comment.
Critical: Coverage threshold set too low.
Setting min_coverage to 0.0 effectively disables coverage checks. This could lead to uncaught regressions and decreased code quality.
Consider maintaining a reasonable coverage threshold (e.g., 80.0 or higher) to ensure code quality:
- min_coverage: 0.0
+ min_coverage: 80.0📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| path: "./coverage/lcov.info" | |
| min_coverage: 0.0 | |
| path: "./coverage/lcov.info" | |
| min_coverage: 80.0 |
.github/workflows/pull-request.yml
Outdated
| env: | ||
| CHANGED_FILES: ${{ steps.changed-files.outputs.all_changed_files }} | ||
| run: npx eslint ${CHANGED_FILES} && python .github/workflows/eslint_disable_check.py |
There was a problem hiding this comment.
Fix shell script quoting to prevent word splitting.
The current implementation is vulnerable to word splitting and globbing issues.
Apply this fix:
- env:
- CHANGED_FILES: ${{ steps.changed-files.outputs.all_changed_files }}
- run: npx eslint ${CHANGED_FILES} && python .github/workflows/eslint_disable_check.py
+ env:
+ CHANGED_FILES: ${{ steps.changed-files.outputs.all_changed_files }}
+ run: npx eslint "${CHANGED_FILES}" && python .github/workflows/eslint_disable_check.py📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| env: | |
| CHANGED_FILES: ${{ steps.changed-files.outputs.all_changed_files }} | |
| run: npx eslint ${CHANGED_FILES} && python .github/workflows/eslint_disable_check.py | |
| env: | |
| CHANGED_FILES: ${{ steps.changed-files.outputs.all_changed_files }} | |
| run: npx eslint "${CHANGED_FILES}" && python .github/workflows/eslint_disable_check.py |
🧰 Tools
🪛 actionlint (1.7.4)
62-62: shellcheck reported issue in this script: SC2086:info:1:12: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 yamllint (1.35.1)
[error] 60-60: trailing spaces
(trailing-spaces)
src/assets/css/app.css
Outdated
| --subtle-blue-grey-hover: #5f7e91; | ||
| --modal-width: 670px; | ||
| --modal-max-width: 680px; | ||
| --input-shadow-color: #dddddd; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding dark mode support for new variables
The new CSS custom properties should have corresponding dark mode values in the [data-bs-theme='dark'] selector to ensure proper theming support.
Add these variables to the dark theme section:
[data-bs-theme='dark'] {
--input-shadow-color: #2d2d2d;
--font-size-header: 16px;
/* Add dark mode values for all new variables */
}Also applies to: 159-159
| .createButton { | ||
| background-color: #a8c7fa !important; | ||
| color: black !important; | ||
| margin: 1rem 0 0; | ||
| margin-top: 15px; | ||
| padding: 10px 10px; | ||
| border-radius: 5px; | ||
| font-size: 16px; | ||
| outline: none; | ||
| font-weight: 600; | ||
| cursor: pointer; | ||
| transition: | ||
| transform 0.2s, | ||
| box-shadow 0.2s; | ||
| width: 100%; | ||
| border: 1px solid var(--dropdown-border-color); | ||
| } | ||
|
|
||
| .createButton:hover { | ||
| background-color: #286fe0 !important; | ||
| color: black !important; | ||
| border: 1px solid var(--dropdown-border-color) !important; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using !important and consider using CSS variables for colors.
The create button styles have several areas for improvement:
- The extensive use of
!importantsuggests specificity issues that should be resolved through better CSS structure - Hardcoded colors should be defined as CSS variables for consistency with the design system
:root {
+ --create-button-bg: #a8c7fa;
+ --create-button-hover-bg: #286fe0;
}
.createButton {
- background-color: #a8c7fa !important;
+ background-color: var(--create-button-bg);
- color: black !important;
+ color: var(--text-color-dark);
}
.createButton:hover {
- background-color: #286fe0 !important;
+ background-color: var(--create-button-hover-bg);
- color: black !important;
+ color: var(--text-color-dark);
- border: 1px solid var(--dropdown-border-color) !important;
+ border: 1px solid var(--dropdown-border-color);
}Committable suggestion skipped: line range outside the PR's diff.
mohamedrehan1
force-pushed
the
develop-postgres
branch
from
753e5b4 to
e05b201
Compare
mohamedrehan1
force-pushed
the
develop-postgres
branch
from
e05b201 to
5ac3c15
Compare
…ation/talawa-admin into develop-postgres
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
src/components/AgendaItems/AgendaItemsContainer.test.tsx (2)
Line range hint
283-286: Standardize event handling approachSome tests mix old and new APIs:
fireEvent.change()anduserEvent.type()are used together- Consider using the new
userEventAPI consistently for all interactionsExample refactor:
-fireEvent.change(title, { target: { value: '' } }); -userEvent.type(title, formData.title); +await user.clear(title); +await user.type(title, formData.title); -fireEvent.change(description, { target: { value: '' } }); -userEvent.type(description, formData.description); +await user.clear(description); +await user.type(description, formData.description);Also applies to: 331-334
Line range hint
447-447: Implement pending drag and drop testsThe comment indicates missing test coverage for drag and drop functionality (line 172-202).
Would you like me to help implement the drag and drop test cases using the new userEvent API? I can provide a complete implementation that follows the updated testing patterns.
src/screens/OrganizationPeople/OrganizationPeople.test.tsx (2)
733-733: Optimize userEvent setupConsider creating a single shared userEvent instance in a beforeEach block instead of creating multiple instances in each test case. This will improve test efficiency.
describe('Organization People Page', () => { + let user; + + beforeEach(() => { + user = userEvent.setup(); + }); test('Testing MEMBERS list', async () => { - const user = userEvent.setup(); // ... rest of the test });Also applies to: 827-827, 887-887, 938-938, 982-982, 1022-1022, 1060-1060, 1127-1127, 1192-1192, 1256-1256, 1296-1296, 1325-1325, 1349-1349, 1367-1367, 1425-1425
Line range hint
992-995: Replace fireEvent with userEventFor consistency and better simulation of real user interactions, replace fireEvent.change with userEvent.type. userEvent provides better simulation of real user behavior.
- fireEvent.change(screen.getByTestId('firstNameInput'), { - target: { value: 'Disha' }, - }); + await user.type(screen.getByTestId('firstNameInput'), 'Disha');Also applies to: 1069-1087, 1136-1154, 1201-1214
src/screens/ManageTag/ManageTag.test.tsx (1)
Line range hint
1-531: Consider restructuring tests for better maintainabilityThe test file could benefit from better organization:
- Group related tests using
describeblocks (e.g., "Modal Interactions", "Navigation", "CRUD Operations")- Extract common setup code into beforeEach blocks
- Consider creating test helpers for common patterns
Example structure:
describe('ManageTag', () => { let user; beforeEach(() => { user = userEvent.setup(); }); describe('Modal Interactions', () => { // Group all modal-related tests }); describe('Navigation', () => { // Group all navigation-related tests }); describe('CRUD Operations', () => { // Group all data manipulation tests }); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (62)
package.json(2 hunks)scripts/custom-test-env.js(1 hunks)src/components/AddOn/core/AddOnRegister/AddOnRegister.test.tsx(3 hunks)src/components/AddOn/core/AddOnStore/AddOnStore.test.tsx(2 hunks)src/components/AddPeopleToTag/AddPeopleToTag.test.tsx(1 hunks)src/components/Advertisements/Advertisements.test.tsx(2 hunks)src/components/Advertisements/core/AdvertisementRegister/AdvertisementRegister.test.tsx(1 hunks)src/components/AgendaCategory/AgendaCategoryContainer.test.tsx(2 hunks)src/components/AgendaItems/AgendaItemsContainer.test.tsx(14 hunks)src/components/DynamicDropDown/DynamicDropDown.test.tsx(3 hunks)src/components/EventListCard/EventListCard.test.tsx(38 hunks)src/components/EventManagement/EventAgendaItems/EventAgendaItems.test.tsx(3 hunks)src/components/EventManagement/EventAttendance/EventAttendance.test.tsx(1 hunks)src/components/EventManagement/EventAttendance/EventStatistics.test.tsx(1 hunks)src/components/EventRegistrantsModal/AddOnSpotAttendee.test.tsx(2 hunks)src/components/LeftDrawer/LeftDrawer.test.tsx(2 hunks)src/components/LeftDrawerOrg/LeftDrawerOrg.test.tsx(1 hunks)src/components/OrgPostCard/OrgPostCard.test.tsx(28 hunks)src/components/OrgSettings/ActionItemCategories/CategoryModal.test.tsx(1 hunks)src/components/OrgSettings/ActionItemCategories/OrgActionItemCategories.test.tsx(4 hunks)src/components/OrgSettings/AgendaItemCategories/AgendaCategoryUpdateModal.test.tsx(2 hunks)src/components/OrgSettings/AgendaItemCategories/OrganizationAgendaCategory.test.tsx(4 hunks)src/components/OrgSettings/General/OrgUpdate/OrgUpdate.test.tsx(1 hunks)src/components/Pagination/Pagination.test.tsx(3 hunks)src/components/ProfileDropdown/ProfileDropdown.test.tsx(3 hunks)src/components/RecurrenceOptions/CustomRecurrence.test.tsx(17 hunks)src/components/RecurrenceOptions/RecurrenceOptions.test.tsx(10 hunks)src/components/UpdateSession/UpdateSession.test.tsx(7 hunks)src/components/UserPasswordUpdate/UserPasswordUpdate.test.tsx(2 hunks)src/components/UserPortal/ChatRoom/ChatRoom.tsx(1 hunks)src/components/UserPortal/CreateDirectChat/CreateDirectChat.test.tsx(3 hunks)src/components/UserPortal/OrganizationNavbar/OrganizationNavbar.test.tsx(6 hunks)src/components/UserPortal/PostCard/PostCard.test.tsx(9 hunks)src/components/UserPortal/Register/Register.test.tsx(4 hunks)src/components/UserPortal/StartPostModal/StartPostModal.test.tsx(2 hunks)src/components/UserPortal/UserNavbar/UserNavbar.test.tsx(7 hunks)src/components/UserPortal/UserSidebarOrg/UserSidebarOrg.test.tsx(1 hunks)src/screens/BlockUser/BlockUser.test.tsx(7 hunks)src/screens/CommunityProfile/CommunityProfile.test.tsx(2 hunks)src/screens/EventManagement/EventManagement.test.tsx(3 hunks)src/screens/EventVolunteers/VolunteerGroups/VolunteerGroupModal.test.tsx(5 hunks)src/screens/ForgotPassword/ForgotPassword.test.tsx(9 hunks)src/screens/FundCampaignPledge/FundCampaignPledge.test.tsx(1 hunks)src/screens/LoginPage/LoginPage.test.tsx(8 hunks)src/screens/ManageTag/ManageTag.test.tsx(20 hunks)src/screens/MemberDetail/MemberDetail.test.tsx(3 hunks)src/screens/OrgList/OrgList.test.tsx(7 hunks)src/screens/OrgPost/OrgPost.test.tsx(16 hunks)src/screens/OrganizationActionItems/ItemModal.test.tsx(3 hunks)src/screens/OrganizationActionItems/OrganizationActionItems.test.tsx(7 hunks)src/screens/OrganizationEvents/OrganizationEvents.test.tsx(9 hunks)src/screens/OrganizationPeople/OrganizationPeople.test.tsx(20 hunks)src/screens/OrganizationTags/OrganizationTags.test.tsx(1 hunks)src/screens/SubTags/SubTags.test.tsx(3 hunks)src/screens/UserPortal/Donate/Donate.test.tsx(2 hunks)src/screens/UserPortal/Events/Events.test.tsx(7 hunks)src/screens/UserPortal/People/People.test.tsx(3 hunks)src/screens/UserPortal/Posts/Posts.test.tsx(4 hunks)src/screens/UserPortal/Settings/Settings.test.tsx(2 hunks)src/screens/UserPortal/Volunteer/Actions/Actions.test.tsx(3 hunks)src/screens/UserPortal/Volunteer/VolunteerManagement.test.tsx(1 hunks)src/setupTests.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (58)
- src/screens/UserPortal/Volunteer/VolunteerManagement.test.tsx
- src/components/UserPortal/OrganizationNavbar/OrganizationNavbar.test.tsx
- src/components/UserPortal/ChatRoom/ChatRoom.tsx
- src/components/Advertisements/core/AdvertisementRegister/AdvertisementRegister.test.tsx
- src/screens/UserPortal/Donate/Donate.test.tsx
- src/components/DynamicDropDown/DynamicDropDown.test.tsx
- src/screens/FundCampaignPledge/FundCampaignPledge.test.tsx
- src/components/AgendaCategory/AgendaCategoryContainer.test.tsx
- src/components/OrgSettings/AgendaItemCategories/AgendaCategoryUpdateModal.test.tsx
- src/components/UserPortal/UserSidebarOrg/UserSidebarOrg.test.tsx
- src/components/UserPortal/StartPostModal/StartPostModal.test.tsx
- src/components/OrgSettings/ActionItemCategories/CategoryModal.test.tsx
- src/components/OrgSettings/General/OrgUpdate/OrgUpdate.test.tsx
- src/components/ProfileDropdown/ProfileDropdown.test.tsx
- src/components/Pagination/Pagination.test.tsx
- src/components/UserPortal/Register/Register.test.tsx
- src/screens/UserPortal/Posts/Posts.test.tsx
- src/screens/BlockUser/BlockUser.test.tsx
- src/components/EventRegistrantsModal/AddOnSpotAttendee.test.tsx
- src/components/LeftDrawer/LeftDrawer.test.tsx
- src/components/AddPeopleToTag/AddPeopleToTag.test.tsx
- src/components/Advertisements/Advertisements.test.tsx
- src/components/RecurrenceOptions/CustomRecurrence.test.tsx
- src/screens/ForgotPassword/ForgotPassword.test.tsx
- src/components/UserPortal/PostCard/PostCard.test.tsx
- src/components/EventManagement/EventAgendaItems/EventAgendaItems.test.tsx
- src/setupTests.ts
- scripts/custom-test-env.js
- src/components/OrgSettings/AgendaItemCategories/OrganizationAgendaCategory.test.tsx
- src/components/OrgPostCard/OrgPostCard.test.tsx
- src/screens/OrganizationTags/OrganizationTags.test.tsx
- src/screens/UserPortal/People/People.test.tsx
- src/screens/UserPortal/Events/Events.test.tsx
- src/components/AddOn/core/AddOnStore/AddOnStore.test.tsx
- src/screens/OrganizationActionItems/OrganizationActionItems.test.tsx
- src/components/UserPasswordUpdate/UserPasswordUpdate.test.tsx
- src/screens/UserPortal/Settings/Settings.test.tsx
- src/screens/OrganizationEvents/OrganizationEvents.test.tsx
- src/screens/SubTags/SubTags.test.tsx
- src/components/UpdateSession/UpdateSession.test.tsx
- src/screens/CommunityProfile/CommunityProfile.test.tsx
- src/components/AddOn/core/AddOnRegister/AddOnRegister.test.tsx
- src/screens/OrganizationActionItems/ItemModal.test.tsx
- src/components/EventManagement/EventAttendance/EventAttendance.test.tsx
- src/screens/UserPortal/Volunteer/Actions/Actions.test.tsx
- src/screens/LoginPage/LoginPage.test.tsx
- src/screens/OrgList/OrgList.test.tsx
- src/components/OrgSettings/ActionItemCategories/OrgActionItemCategories.test.tsx
- package.json
- src/components/RecurrenceOptions/RecurrenceOptions.test.tsx
- src/components/LeftDrawerOrg/LeftDrawerOrg.test.tsx
- src/screens/OrgPost/OrgPost.test.tsx
- src/components/UserPortal/CreateDirectChat/CreateDirectChat.test.tsx
- src/components/UserPortal/UserNavbar/UserNavbar.test.tsx
- src/screens/MemberDetail/MemberDetail.test.tsx
- src/components/EventListCard/EventListCard.test.tsx
- src/components/EventManagement/EventAttendance/EventStatistics.test.tsx
- src/screens/EventVolunteers/VolunteerGroups/VolunteerGroupModal.test.tsx
🔇 Additional comments (11)
src/components/AgendaItems/AgendaItemsContainer.test.tsx (2)
34-41: LGTM: Improved type safety in console error handling
The addition of type checking for the console message improves type safety while properly filtering React 18's defaultProps deprecation warnings.
Line range hint 1-447: Overall implementation aligns with @testing-library/user-event v14 upgrade
The changes successfully migrate most user interactions to the new async API required by @testing-library/user-event v14. The implementation is solid, with only minor consistency improvements suggested above.
src/screens/OrganizationPeople/OrganizationPeople.test.tsx (2)
735-736: Fix async forEach loops
The forEach loops with async callbacks won't work as expected since forEach doesn't wait for async operations.
Also applies to: 828-829, 1258-1259, 1368-1369
Line range hint 836-840: Uncomment and fix table fetching assertions
There are commented assertions related to table fetching functionality. These should be uncommented and fixed once the underlying functionality is working.
src/screens/EventManagement/EventManagement.test.tsx (4)
68-68:
Use userEvent.setup() for back button click
The change to fireEvent reduces test fidelity. Navigation tests especially benefit from proper browser event simulation.
-fireEvent.click(backButton);
+const user = userEvent.setup();
+await user.click(backButton);3-3:
Restore userEvent import and upgrade to v14.5.2
This change contradicts the PR objectives. Instead of removing userEvent, we should upgrade it to v14.5.2 as specified in the PR description. The userEvent library provides better browser simulation than fireEvent.
Apply this diff:
-import { fireEvent, render, screen, waitFor } from '@testing-library/react';
+import { render, screen, waitFor } from '@testing-library/react';
+import userEvent from '@testing-library/user-event';Likely invalid or redundant comment.
79-106: 🛠️ Refactor suggestion
Refactor tab switching test to use userEvent.setup()
Multiple instances of fireEvent should be replaced with userEvent for better browser simulation. The userEvent setup can be shared across all clicks in this test.
Apply this refactor:
test('Testing event management tab switching', async () => {
renderEventManagement();
+ const user = userEvent.setup();
const registrantsButton = screen.getByTestId('registrantsBtn');
- fireEvent.click(registrantsButton);
+ await user.click(registrantsButton);
const registrantsTab = screen.getByTestId('eventRegistrantsTab');
expect(registrantsTab).toBeInTheDocument();
const eventAttendanceButton = screen.getByTestId('attendanceBtn');
- fireEvent.click(eventAttendanceButton);
+ await user.click(eventAttendanceButton);
// ... similar changes for all other click events ...
const volunteerButton = screen.getByTestId('volunteersBtn');
- fireEvent.click(volunteerButton);
+ await user.click(volunteerButton);Note: Don't forget to mark the test function as async and await all user interactions.
Likely invalid or redundant comment.
Line range hint 1-141: Verify similar changes in related test files
Since this change affects test timing and event handling, we should verify similar patterns in other test files.
src/screens/ManageTag/ManageTag.test.tsx (3)
148-150: Systematically update test patterns across all modal tests
The same issues appear across multiple test cases:
- Unnecessary
act()wrappers aroundrenderManageTag - Outdated userEvent usage without setup
Apply the same fixes as suggested in the previous comment across all these test cases:
- Remove act() wrapper
- Setup userEvent
- Await user interactions
Also applies to: 172-174, 199-201, 226-228, 250-252, 274-276, 291-293, 308-310, 327-329, 344-346, 367-369
108-112: 🛠️ Refactor suggestion
Simplify async rendering pattern
The act() wrapper around renderManageTag is unnecessary as React Testing Library's render function already handles this internally. The use of findByText is good for async rendering.
- const { findByText } = await act(() => renderManageTag(link));
+ const { findByText } = renderManageTag(link);Likely invalid or redundant comment.
126-138:
Update userEvent implementation to v14 pattern
The test needs several improvements:
- Remove unnecessary
act()wrapper - Setup userEvent before use (required in v14)
- Await userEvent interactions
- await act(async () => {
- renderManageTag(link);
- });
+ const user = userEvent.setup();
+ renderManageTag(link);
expect(await screen.findByTestId('addPeopleToTagBtn')).toBeInTheDocument();
- userEvent.click(await screen.findByTestId('addPeopleToTagBtn'));
+ await user.click(await screen.findByTestId('addPeopleToTagBtn'));
expect(
await screen.findByTestId('addPeopleToTagModal'),
).toBeInTheDocument();
- userEvent.click(await screen.findByTestId('closeAddPeopleToTagModal'));
+ await user.click(await screen.findByTestId('closeAddPeopleToTagModal'));Likely invalid or redundant comment.
| const memebersDropdownItem = await screen.findByTestId('members'); | ||
| await user.click(memebersDropdownItem); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace wait() with findBy queries
Instead of using wait() after user interactions, use findBy queries to wait for specific conditions. This makes tests more reliable and explicit about what they're waiting for.
- await user.click(memebersDropdownItem);
- await wait();
+ await user.click(await screen.findByTestId('members'));Also applies to: 832-833, 892-892, 1027-1027, 1065-1065, 1372-1373
| @@ -443,7 +465,7 @@ describe('Manage Tag Page', () => { | |||
| }); | |||
| userEvent.click(screen.getAllByTestId('unassignTagBtn')[0]); | |||
|
|
|||
| userEvent.click(screen.getByTestId('unassignTagModalSubmitBtn')); | |||
| userEvent.click(await screen.findByTestId('unassignTagModalSubmitBtn')); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Standardize event handling patterns
The tests mix fireEvent and userEvent, which is not recommended. Additionally, some user interactions are not properly awaited.
- Standardize on
userEventfor user interactions - Ensure all user interactions are properly awaited
Example fix:
- fireEvent.change(input, { target: { value: 'assigned user' } });
+ await user.type(input, 'assigned user');
- userEvent.click(screen.getByTestId('editTagSubmitBtn'));
+ await user.click(await screen.findByTestId('editTagSubmitBtn'));Also applies to: 489-489, 521-521, 529-531
Description
Updated project dependencies and configurations to their latest stable versions. Key updates include:
Fixes #2072
Testing Library Updates
Test Fixes & Updates
Summary by CodeRabbit
New Features
ChatRoomcomponent with optional chaining.Bug Fixes
Documentation
Refactor
userEventtofireEventin specific test cases for better alignment with expected behaviors.Chores