Fixes: #2071 - Misplaced Screen Title & Fix Layout Structure #2356
Conversation
WalkthroughThe pull request includes updates to the Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 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 ProcessWe have these basic policies to make the approval process smoother for our volunteer team. Testing Your CodePlease make sure your code passes all tests. Our test code coverage system will fail if these conditions occur:
The process helps maintain the overall reliability of the code base and is a prerequisite for getting your PR approved. Assigned reviewers regularly review the PR queue and tend to focus on PRs that are passing. 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: 3
🧹 Outside diff range and nitpick comments (2)
src/screens/UserPortal/UserScreen/UserScreen.tsx (1)
133-134: Remove redundant commentsThe before/after comments don't provide any value and should be removed for cleaner code.
package.json (1)
49-49: Document the reason for the dependency update.Please add a note in the PR description explaining why this react-icons update was necessary for the post title fix. This will help with future maintenance and understanding of the changes.
📜 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 (3)
- package.json (1 hunks)
- src/screens/UserPortal/Posts/Posts.tsx (2 hunks)
- src/screens/UserPortal/UserScreen/UserScreen.tsx (1 hunks)
🔇 Additional comments (3)
package.json (1)
49-49: Verify if this dependency update is required.The update of react-icons from ^5.2.1 to ^5.3.0 appears to be a safe minor version bump. However, since the PR's main objective is to fix the misplaced post title issue, please verify if this update is necessary for the layout changes.
src/screens/UserPortal/Posts/Posts.tsx (2)
211-215: LGTM! Improved mapping logic for likes.The change to using
mapinstead of mutation-based approach is a good improvement for readability and maintainability.
Line range hint
1-400: Verify title removal aligns with parent container implementation.The removal of the screen title from this component aligns with the PR objective of moving titles to the Parent Outlet Container. However, we should verify that the parent container properly displays the title for this route.
✅ Verification successful
Title handling is properly implemented in the parent container
The title removal from Posts.tsx is correct as the parent container (UserScreen.tsx) already handles the title display. The code shows:
- UserScreen.tsx maps routes to title keys and renders the title in its layout:
const titleKey: string | undefined = map[location.pathname.split('/')[2]]; // ... <h1>{titleKey !== 'home' ? t('title') : 'Posts'}</h1>
- The title specifically handles the 'home' route (Posts screen) by displaying "Posts" and other routes by using translated titles.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the parent container implementation rg -l "Parent.*Outlet.*Container" --type typescriptLength of output: 88
Script:
#!/bin/bash # Search for files containing "Outlet" component usage rg -l "Outlet" # Search for layout or parent container components rg -l "Layout|Container" --type ts --type tsxLength of output: 425
Script:
#!/bin/bash # Check the UserScreen component since it's in the same directory hierarchy cat src/screens/UserPortal/UserScreen/UserScreen.tsx # Check SecuredRouteForUser since it's in the UserPortal context cat src/components/UserPortal/SecuredRouteForUser/SecuredRouteForUser.tsxLength of output: 5544
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- src/screens/UserPortal/Posts/Posts.tsx (2 hunks)
- src/screens/UserPortal/UserScreen/UserScreen.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/screens/UserPortal/UserScreen/UserScreen.tsx
🔇 Additional comments (1)
src/screens/UserPortal/Posts/Posts.tsx (1)
Line range hint
1-400: Title changes align with PR objectives.The removal of the screen title from this child container aligns with the PR objective of moving titles to the Parent Outlet Container.
palisadoes
changed the title
Okay |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2356 +/- ##
===========================================
- Coverage 98.31% 98.30% -0.02%
===========================================
Files 283 283
Lines 8207 8200 -7
Branches 2351 2382 +31
===========================================
- Hits 8069 8061 -8
- Misses 129 130 +1
Partials 9 9 ☔ View full report in Codecov by Sentry. |
i have fixed the failed tests issue. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/screens/UserPortal/UserScreen/UserScreen.test.tsx (2)
Line range hint 113-143: Consider improving the test case name for clarity.
The current test name "Testing LeftDrawer in page functionality" could be more descriptive. Consider renaming it to something like "LeftDrawer should toggle correctly based on window size and user interaction" to better reflect what's being tested.
- test('Testing LeftDrawer in page functionality', async () => {
+ test('LeftDrawer should toggle correctly based on window size and user interaction', async () => {Line range hint 145-161: Consider using React Router's utilities for navigation testing.
While the test correctly verifies the redirection behavior, using window.location directly can be fragile. React Router provides utilities specifically designed for testing navigation.
Consider refactoring to use React Router's testing utilities:
- test('should be redirected to / if orgId is undefined', async () => {
+ test('should be redirected to root when orgId is undefined', async () => {
mockID = undefined;
+ const navigate = jest.fn();
+ jest.spyOn(require('react-router-dom'), 'useNavigate').mockReturnValue(navigate);
render(
<MockedProvider addTypename={false} link={link}>
<BrowserRouter>
<Provider store={store}>
<I18nextProvider i18n={i18nForTest}>
<UserScreen />
</I18nextProvider>
</Provider>
</BrowserRouter>
</MockedProvider>,
);
- expect(window.location.pathname).toEqual('/');
+ expect(navigate).toHaveBeenCalledWith('/');
});📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- src/GraphQl/Mutations/OrganizationMutations.ts (0 hunks)
- src/screens/UserPortal/UserScreen/UserScreen.test.tsx (1 hunks)
💤 Files with no reviewable changes (1)
- src/GraphQl/Mutations/OrganizationMutations.ts
🔇 Additional comments (3)
src/screens/UserPortal/UserScreen/UserScreen.test.tsx (3)
90-90: LGTM! Test assertion aligns with the new title rendering behavior.
The updated assertion correctly verifies that the screen title is now properly displayed as 'Posts', which aligns with the PR's objective of fixing the screen title placement and rendering.
Line range hint 92-111: LGTM! Good coverage of dynamic title rendering.
The test case effectively verifies that screen titles are correctly rendered based on different routes, which is crucial for validating the title placement fix across various screens in the User Portal.
Line range hint 1-161: Verify test coverage for all title-related edge cases.
While the current tests cover the main scenarios, consider adding test cases for:
- Routes that don't have a corresponding title mapping
- Title updates when navigating between routes
- Special characters or long titles in different locales
|
Please fix the failing Code Coverage test. |
Did the recommended changes. |
|
Please fix the failing tests |
Yes |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (8)
src/components/UserPortal/CreateDirectChat/CreateDirectChat.tsx (1)
Line range hint
119-124: Enhance accessibility for modal and table elements.Consider the following accessibility improvements:
- Add
aria-labelledbyto the modal- Translate the search input placeholder
- Add meaningful aria-labels to table headers
- <Modal + <Modal + aria-labelledby="direct-chat-modal-title" data-testid="createDirectChatModal" show={createDirectChatModalisOpen} onHide={toggleCreateDirectChatModal} contentClassName={styles.modalContent} > <Modal.Header closeButton data-testid="createDirectChat"> - <Modal.Title>{'Chat'}</Modal.Title> + <Modal.Title id="direct-chat-modal-title">{'Chat'}</Modal.Title> </Modal.Header> // ... later in the code ... <TableHead> <TableRow> - <StyledTableCell>#</StyledTableCell> + <StyledTableCell aria-label="Index">#</StyledTableCell> - <StyledTableCell align="center">{'user'}</StyledTableCell> + <StyledTableCell align="center" aria-label="User Details">{'user'}</StyledTableCell> - <StyledTableCell align="center">{'Chat'}</StyledTableCell> + <StyledTableCell align="center" aria-label="Chat Actions">{'Chat'}</StyledTableCell> </TableRow> </TableHead>Also applies to: 144-150
src/components/UserPortal/OrganizationCard/OrganizationCard.test.tsx (1)
Line range hint
108-114: Consider usingwaitForinstead of custom wait function.The current implementation uses a fixed delay which could make tests fragile. Consider using
@testing-library/react's built-inwaitForutility for more reliable async testing:-async function wait(ms = 100): Promise<void> { - await act(() => { - return new Promise((resolve) => { - setTimeout(resolve, ms); - }); - }); -}Replace wait calls with:
import { waitFor } from '@testing-library/react'; // Usage await waitFor(() => { // assertions here });This approach is more robust as it automatically retries assertions until they pass or timeout.
src/components/UserPortal/CreateDirectChat/CreateDirectChat.test.tsx (3)
Line range hint
1-800: Improve mock data consistency and maintainability.Consider the following improvements to the mock data:
- Use ISO date strings instead of numeric strings for timestamps
- Standardize ID formats across mock objects
- Extract common mock data into shared fixtures
Example refactor for timestamps:
-createdAt: '2345678903456', +createdAt: '2024-01-15T10:30:00.000Z',
Line range hint
801-810: Consider making wait utility more flexible.The wait utility could be more configurable by:
- Adding a default export
- Using a constant for the default delay
+const DEFAULT_WAIT_DELAY = 100; -async function wait(ms = 100): Promise<void> { +export default async function wait(ms = DEFAULT_WAIT_DELAY): Promise<void> { await act(() => { return new Promise((resolve) => { setTimeout(resolve, ms); }); }); }
Line range hint
811-940: Enhance test coverage with additional test cases.The current test suite is missing coverage for:
- Error handling (network errors, validation errors)
- Search functionality validation
- Empty states and edge cases
- Modal content verification
Consider adding these test cases:
test('should handle network error when creating chat', async () => { // Mock with network error // Verify error handling }); test('should validate search results', async () => { // Test search functionality // Verify filtered results }); test('should handle empty search results', async () => { // Test with no matching users // Verify empty state }); test('should validate modal content', async () => { // Verify modal title // Verify form elements // Verify button states });src/screens/UserPortal/Chat/Chat.test.tsx (3)
Line range hint
1548-1587: Enhance test coverage for direct chat creation.The current test only verifies the presence of UI elements without testing the actual chat creation functionality. Consider adding test cases for:
- Successful chat creation with form submission
- Error handling scenarios
- Validation of created chat data
Example test structure:
test('create new direct chat', async () => { // ... existing setup ... // Test form submission const nameInput = screen.getByTestId('chatName'); fireEvent.change(nameInput, { target: { value: 'Test Chat' } }); // Select a contact const contactSelect = screen.getByTestId('contactSelect'); fireEvent.click(contactSelect); // Submit and verify fireEvent.click(submitBtn); expect(await screen.findByText('Chat created successfully')).toBeInTheDocument(); // Verify error case fireEvent.click(submitBtn); expect(await screen.findByText('Error creating chat')).toBeInTheDocument(); });
Line range hint
1588-1631: Improve group chat creation test coverage.The test only verifies basic UI rendering without testing group-specific functionality. Consider adding:
- Group name validation
- Member selection and validation
- Group size limits
- Error scenarios
Example implementation:
test('create new group chat', async () => { // ... existing setup ... // Test group name validation const nameInput = screen.getByTestId('groupName'); fireEvent.change(nameInput, { target: { value: '' } }); expect(screen.getByText('Group name is required')).toBeInTheDocument(); // Test member selection const memberSelect = screen.getByTestId('memberSelect'); fireEvent.click(memberSelect); const members = await screen.findAllByTestId('member-option'); fireEvent.click(members[0]); // Test minimum member validation fireEvent.click(submitBtn); expect(screen.getByText('Minimum 2 members required')).toBeInTheDocument(); });
Line range hint
1632-1722: Clean up and enhance sidebar tests.A few improvements needed for the sidebar tests:
- Remove
screen.debug()as it's typically used for development- Enhance error message for better debugging
- Add more test cases for different screen sizes
Apply these changes:
- screen.debug(); - throw new Error('Close menu button not found'); + throw new Error('Close menu button not found. This might indicate a rendering issue with the sidebar component or a timing problem with the test.'); // Add comprehensive resize tests test('Testing sidebar at various screen sizes', async () => { const screenSizes = [400, 820, 1024]; for (const size of screenSizes) { resizeWindow(size); // Test sidebar behavior } });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
src/components/UserPortal/CreateDirectChat/CreateDirectChat.test.tsx(1 hunks)src/components/UserPortal/CreateDirectChat/CreateDirectChat.tsx(2 hunks)src/components/UserPortal/CreateGroupChat/CreateGroupChat.tsx(2 hunks)src/components/UserPortal/OrganizationCard/OrganizationCard.test.tsx(1 hunks)src/components/UserPortal/UserSidebar/UserSidebar.test.tsx(1 hunks)src/screens/UserPortal/Chat/Chat.test.tsx(2 hunks)src/screens/UserPortal/UserScreen/UserScreen.test.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/screens/UserPortal/UserScreen/UserScreen.test.tsx
🔇 Additional comments (5)
src/components/UserPortal/OrganizationCard/OrganizationCard.test.tsx (2)
2-2: LGTM! Clean import consolidation.
The consolidation of testing library imports improves code organization.
Line range hint 142-270: Consider adding layout structure tests.
Given that this PR addresses screen title placement and layout structure issues, consider adding test cases to verify:
- The correct positioning of components within the card
- The absence of unnecessary empty spaces
- Proper alignment with the parent container
This would help ensure the layout fixes are maintained.
src/components/UserPortal/UserSidebar/UserSidebar.test.tsx (2)
3-3: LGTM: Removed unused import.
The removal of the unused waitFor import is appropriate as the test suite successfully handles asynchronous operations using act and the custom wait function.
1-1: Consider adding tests for screen title placement.
While the UserSidebar tests are comprehensive, the PR's main objective of fixing screen title placement (moving from child containers to Parent Outlet Container) isn't covered in the test suite. Consider adding test cases to verify the correct positioning of screen titles in the appropriate test file (e.g., UserScreen.test.tsx).
Let's check if there are any existing tests for screen title placement:
Would you like help creating test cases to verify the screen title placement?
src/components/UserPortal/CreateDirectChat/CreateDirectChat.test.tsx (1)
Line range hint 811-940: Verify screen title placement changes.
The test suite should include specific tests to verify the screen title placement fixes mentioned in PR #2071.
Would you like me to help create test cases specifically for verifying the screen title placement fixes?
There was a problem hiding this comment.
Make sure code rabbit approves your changes.
Please fix the failing tests. Our test code coverage system will fail if any of these conditions occur:
- The overall code coverage drops below the target threshold of the repository
- Any file in the pull request has code coverage levels below the repository threshold
- The code does not comply with our linting check or other related formatting requirements
We do this to ensure the overall reliability of the code base is maintained. This is a prerequisite for getting your PR approved. Assigned reviewers regularly review the PR queue and tend to focus on PRs that are passing.
|
Hey @VanshikaSabharwal , Please include a screenshot or video of the result. This is a frontend issue, so it would be easier to address with a visual representation rather than code snippets. We can already see the modified files. |
Okay I'll keep this in mind |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
schema.graphql(6 hunks)
🔇 Additional comments (5)
schema.graphql (5)
697-702: LGTM! Well-structured mutation definition.
The sendMessageToChat mutation is properly formatted with parameters on separate lines, improving readability. The optional replyTo parameter correctly allows for message threading.
1033-1038: LGTM! Standard connection pattern implementation.
The advertisementsConnection query follows the Relay connection specification with proper pagination parameters.
1095-1095: LGTM! Consistent query definition.
The venue query is properly typed and consistent with the Venue type definition.
1398-1398: LGTM! Required field properly typed.
The selectedOrganization field in UserInput is correctly typed as a required ID.
1544-1544: LGTM! Well-structured input type.
The chatInput type contains all necessary fields for chat creation with proper typing.
|
Hey @pranshugupta54 . I tried to solve as much failing tests as possible but i feel stuck now. Can you or anyone help me?? |
|
@VanshikaSabharwal, this PR is not even solving the issue at all. Please read the issue again. |
@pranshugupta54 It does fix the issue. see |
|
Okay, does it work for all pages? |
| @@ -138,7 +138,7 @@ | |||
| "jest": "^27.4.5", | |||
| "jest-localstorage-mock": "^2.4.19", | |||
| "jest-location-mock": "^2.0.0", | |||
| "jest-preview": "^0.3.1", | |||
| "jest-preview": "^0.2.3", | |||
There was a problem hiding this comment.
We don't need this change here
There was a problem hiding this comment.
I did not made any changes here see.
There was a problem hiding this comment.
It's 0.3.1 on develop and your PR is making it 0.2.3
There was a problem hiding this comment.
Okay, got it
package-lock.json
Outdated
There was a problem hiding this comment.
Revert the changes in this file
There was a problem hiding this comment.
I reverted the changes and updated the package.json according to current code of develop branch.
There was a problem hiding this comment.
This file shouldn't require any change too
There was a problem hiding this comment.
i was trying to pass the failed tests. so, i did changes thinking it would pass tests but still the tests failed. So i think i should revert changes
Runs Introspection on the GitHub talawa-api repo on the schema.graphql file
failed 6 hours ago in 25s
There was a problem hiding this comment.
i reverted all changes.
Yes, It work for all pages |
|
@VanshikaSabharwal Can you open a new PR? I think there are more unrelated changes of what actually needed maybe it due to eslint errors or something. It's getting hard to review this PR. But remember eslint errors occurs only to your code that modified and can be safely mitigated for other unrelated code. The introspection workflow may fail due to other prs too. |
|
Reach out on Slack if you got stuck anywhere |
What kind of change does this PR introduce?
bugfix
Issue Number:
Fixes #2071
Did you add tests for your changes?
No
Snapshots/Videos:
Summary
Does this PR introduce a breaking change?
No
Other information
Have you read the contributing guide?
Yes
Summary by CodeRabbit
UserScreencomponent.UserScreen,Chat, andOrganizationCardcomponents.Posts,CreateDirectChat, andCreateGroupChatcomponents.