Fix Misplaced Screen Title & Fix Layout Structure #2412
Conversation
WalkthroughThe changes in this pull request primarily involve the removal of the Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Our Pull Request Approval 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
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2412 +/- ##
===========================================
- Coverage 98.28% 98.28% -0.01%
===========================================
Files 283 283
Lines 8229 8221 -8
Branches 2352 2383 +31
===========================================
- Hits 8088 8080 -8
Misses 132 132
Partials 9 9 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (18)
src/screens/UserPortal/UserScreen/UserScreen.tsx (1)
Route to title mapping needs to be updated
The current mapping is incomplete. Based on the routes found in
App.tsxand the existingmapobject inUserScreen.tsx, these routes are missing from the mapping:
/user/organizations/user/settings/user/chat/user/event/:orgId/:eventIdThe
mapobject is used to determine translation keys based on the second segment of the URL path (location.pathname.split('/')[2]), and missing mappings will result in undefined translation keys.🔗 Analysis chain
Line range hint
19-27: Verify route to title mapping.The
mapobject is crucial for determining the correct translation key. Ensure it covers all possible routes and their corresponding title keys.Let's check for any unmapped routes:
Let me gather more context about the existing route mapping by checking the component code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all possible routes that might need titles # Look for route definitions that might need to be included in the map rg -g '*.tsx' -g '*.ts' "path.*user/[a-zA-Z]+"Length of output: 4773
Script:
#!/bin/bash # Get the full content of UserScreen.tsx to understand the route mapping implementation cat src/screens/UserPortal/UserScreen/UserScreen.tsxLength of output: 4204
src/screens/UserPortal/UserScreen/UserScreen.test.tsx (4)
90-90: Consider making the test description more specific.The test name "renders the correct title based on the titleKey for posts" could be more descriptive about the expected behavior.
- test('renders the correct title based on the titleKey for posts', () => { + test('should render "Posts" title when viewing the posts screen', () => {
Line range hint
94-111: Ensure proper test isolation.The test modifies the shared
mockLocationvariable but doesn't reset it afterward, which could affect other tests. Consider usingbeforeEach/afterEachhooks for proper test isolation.+beforeEach(() => { + mockLocation = '/user/organization/123'; +}); test('renders the correct title based on the titleKey', () => { mockLocation = '/user/people/123'; // ... rest of the test });
Line range hint
112-140: Enhance test maintainability and readability.Consider these improvements:
- Add window size cleanup after test
- Use constants for window sizes
- Add more descriptive assertion messages
+const MOBILE_WIDTH = 800; +const DESKTOP_WIDTH = 1000; test('LeftDrawer should toggle correctly based on window size and user interaction', async () => { // ... setup code ... - resizeWindow(800); + resizeWindow(MOBILE_WIDTH); clickToggleMenuBtn(toggleButton); - expect(icon).toHaveClass('fa fa-angle-double-left'); + expect(icon).toHaveClass('fa fa-angle-double-left', 'Icon should indicate collapsed state on mobile'); - resizeWindow(1000); + resizeWindow(DESKTOP_WIDTH); // ... rest of the test ... + // Cleanup + resizeWindow(window.innerWidth); });
Line range hint
141-156: Fix critical issues in navigation test implementation.The test has several issues:
- The useNavigate mock implementation is incorrect
- The assertion should check the navigate function call instead of window.location
- Missing cleanup of mockID
+beforeEach(() => { + mockID = '123'; +}); test('should be redirected to root when orgId is undefined', async () => { mockID = undefined; const navigate = jest.fn(); - jest.spyOn({ useNavigate }, 'useNavigate').mockReturnValue(navigate); + (useNavigate as jest.Mock).mockReturnValue(navigate); render( // ... component render ... ); - expect(window.location.pathname).toEqual('/'); + expect(navigate).toHaveBeenCalledWith('/'); });package.json (1)
11-12: Consider pinning exact versions.Using caret (^) for version ranges can lead to unexpected behavior if patches include breaking changes. Consider pinning exact versions for more predictable builds:
- "@dicebear/collection": "^9.2.2", - "@dicebear/core": "^9.2.2", + "@dicebear/collection": "9.2.2", + "@dicebear/core": "9.2.2",src/components/UserPortal/CreateDirectChat/CreateDirectChat.tsx (4)
64-66: Remove commented-out code.These commented-out lines should be removed if the translation functionality is no longer needed.
Line range hint
75-89: Add error handling and loading state for chat creation.The
handleCreateDirectChatfunction should:
- Handle potential errors from the mutation
- Show a loading state during creation
- Provide feedback on success/failure
Consider updating the implementation:
const handleCreateDirectChat = async (id: string): Promise<void> => { + try { + setIsCreating(true); await createChat({ variables: { organizationId, userIds: [userId, id], isGroup: false, }, }); await chatsListRefetch(); toggleCreateDirectChatModal(); + // Show success message + } catch (error) { + // Show error message + } finally { + setIsCreating(false); + } };
Line range hint
98-112: Improve search functionality implementation.The current search implementation has limitations:
- Splitting by space assumes a perfect "firstName lastName" format
- Doesn't handle multiple spaces or middle names
- Performs exact matches which might not be user-friendly
Consider using a single search field that searches across both first and last names, or implement a more robust name parsing logic.
Line range hint
116-190: Consider consolidating UI libraries.The component mixes Material-UI and React-Bootstrap components, which can lead to:
- Inconsistent styling and behavior
- Increased bundle size
- Maintenance complexity
Consider standardizing on one UI library throughout the application for better consistency and maintainability.
src/screens/UserPortal/Posts/Posts.tsx (1)
211-229: Consider enhancing type safety and readability.While the null handling is good, we can improve the code further:
Consider this refactoring for better type safety and readability:
- const allLikes: InterfacePostLikes = likedBy.map((value) => ({ - firstName: value.firstName, - lastName: value.lastName, - id: value._id, - })); + const allLikes = likedBy.map((value): InterfacePostLikes[number] => ({ + firstName: value.firstName, + lastName: value.lastName, + id: value._id, + })); - const postComments: InterfacePostComments = - comments?.map((value) => ({ - id: value.id, - creator: { - firstName: value.creator?.firstName ?? '', - lastName: value.creator?.lastName ?? '', - id: value.creator?.id ?? '', - email: value.creator?.email ?? '', - }, - likeCount: value.likeCount, - likedBy: value.likedBy?.map((like) => ({ id: like?.id ?? '' })) ?? [], - text: value.text, - })) ?? []; + const postComments = comments?.map((value): InterfacePostComments[number] => { + const creator = value.creator ?? {}; + return { + id: value.id, + creator: { + firstName: creator.firstName ?? '', + lastName: creator.lastName ?? '', + id: creator.id ?? '', + email: creator.email ?? '', + }, + likeCount: value.likeCount, + likedBy: value.likedBy?.map((like) => ({ id: like?.id ?? '' })) ?? [], + text: value.text, + }; + }) ?? [];Changes:
- Added explicit type annotations in map callbacks
- Destructured
creatorto reduce nesting- Improved code structure for better readability
src/components/UserPortal/UserSidebar/UserSidebar.test.tsx (1)
3-3: Consider keeping thewaitForutility.While removing
waitForin favor of a customwaitfunction works, the@testing-library/react'swaitForutility is specifically designed for testing async operations and provides better error messages and debugging capabilities.-import { fireEvent, render, screen } from '@testing-library/react'; +import { fireEvent, render, screen, waitFor } from '@testing-library/react';schema.graphql (3)
1095-1095: Consider adding filtering options to venue query.While the basic query structure is correct, consider adding filtering capabilities similar to other queries in the schema (e.g.,
whereinput type) to make it more consistent with the rest of the API.- venue(id: ID!): [Venue] + venue( + id: ID! + where: VenueWhereInput + ): [Venue]Consider adding this input type:
input VenueWhereInput { name_contains: String capacity_gte: Int capacity_lte: Int }
1544-1544: Consider adding validation constraints for chat creation.While the
chatInputtype contains the necessary fields, consider adding validation constraints:
- For group chats, name should be required
- Minimum number of users for group/direct chats
input chatInput { isGroup: Boolean! - organizationId: ID + organizationId: ID! userIds: [ID!]! - name: String + name: String @constraint( + minLength: 1 + maxLength: 50 + required: true + requiredIf: "isGroup" + ) }
Line range hint
1-1544: Consider implementing consistent pagination pattern across queries.Some queries use offset-based pagination (skip/first), while others use cursor-based pagination (after/before/first/last). Consider standardizing the pagination approach across all connection types for consistency.
src/components/UserPortal/CreateDirectChat/CreateDirectChat.test.tsx (2)
Line range hint
832-838: Consider enhancing the wait utility functionThe wait utility function could be improved for better maintainability and reliability:
- Add JSDoc documentation
- Consider increasing the default timeout
- Add type safety for the return value
+/** + * Utility function to wait for async operations in tests + * @param ms - The number of milliseconds to wait (default: 500) + * @returns Promise<void> + */ -async function wait(ms = 100): Promise<void> { +async function wait(ms = 500): Promise<void> { await act(() => { return new Promise((resolve) => { setTimeout(resolve, ms); }); }); }
Line range hint
840-957: Enhance test coverage for edge casesThe current test suite covers basic functionality but would benefit from additional test cases:
- Error handling scenarios (network errors, validation errors)
- User search functionality validation
- Empty states and loading states
- Edge cases for user selection
Here's an example of additional test cases to consider:
test('should handle network error when creating chat', async () => { // Mock implementation with network error }); test('should handle empty user search results', async () => { // Test empty state rendering }); test('should show loading state while searching users', async () => { // Test loading indicator }); test('should validate minimum/maximum user selection', async () => { // Test user selection constraints });src/screens/UserPortal/Chat/Chat.test.tsx (1)
Line range hint
1588-1677: Consider enhancing test coverage for group chat and sidebar.While the basic functionality is tested, consider adding these scenarios:
- Group chat creation with multiple users
- Form validation for group name
- Sidebar state persistence across renders
- Edge cases for different screen sizes
Here's a suggested addition to improve the group chat test:
test('create new group chat', async () => { // ... existing setup ... fireEvent.click(newGroupChatBtn); + // Test form validation + const submitButton = screen.getByTestId('submitBtn'); + fireEvent.click(submitButton); + expect(screen.getByText(/group name is required/i)).toBeInTheDocument(); + + // Test adding multiple users + const userInput = screen.getByTestId('userSelect'); + fireEvent.change(userInput, { target: { value: 'Test User' } }); + const userOption = await screen.findByText('Test User'); + fireEvent.click(userOption); + + // Test group creation + const groupNameInput = screen.getByTestId('groupName'); + fireEvent.change(groupNameInput, { target: { value: 'New Group' } }); + fireEvent.click(submitButton); + await waitFor(() => { + expect(screen.queryByRole('dialog')).not.toBeInTheDocument(); + }); // ... existing cleanup ... });
🛑 Comments failed to post (2)
package.json (1)
11-12: 💡 Codebase verification
Breaking changes detected in dicebear upgrade
The upgrade from v8 to v9 of @dicebear packages requires attention:
- Found active usage in
src/components/Avatar/Avatar.tsxwhich imports both@dicebear/coreand@dicebear/collection- Multiple test files reference dicebear URLs that may need updates (currently using v5.x API endpoints)
- Git history shows multiple major version jumps (v7 → v8 → v9) with breaking changes
Please ensure:
- Update API endpoint URLs in tests from
api.dicebear.com/5.xto the current version- Review dicebear v9 migration guide for breaking changes
- Update avatar implementation in
Avatar.tsxaccording to v9 API changes🔗 Analysis chain
Major version upgrade requires careful review.
The upgrade from v8 to v9 of @dicebear packages represents a major version change that may include breaking changes.
Let's verify the changelog and potential breaking changes:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for breaking changes in @dicebear packages # Test 1: Look for package.json files that might be using these dependencies echo "Searching for other package.json files using @dicebear..." fd package.json --exec grep -l "@dicebear" # Test 2: Look for actual usage of @dicebear in the codebase echo "Checking usage of @dicebear in the codebase..." rg "@dicebear" --type ts --type tsxLength of output: 349
Script:
#!/bin/bash # Let's try a different approach to find @dicebear usage # Search for any files containing dicebear (case-insensitive) echo "Searching for files containing dicebear..." rg -i "dicebear" --type js --type jsx --type ts # Look for imports/requires of dicebear packages echo "Searching for specific dicebear imports..." ast-grep --pattern 'import $_ from "@dicebear/$_"' # Check package-lock.json for current version echo "Checking package-lock.json for version info..." rg "\"@dicebear/" package-lock.json -A 3Length of output: 4907
Script:
#!/bin/bash # Let's search for files with common extensions that might use dicebear echo "Searching for files containing dicebear in source files..." rg -i "dicebear" -g "*.{js,jsx,ts,tsx,vue}" # Look for any avatar-related components that might use dicebear echo "Searching for avatar-related files..." fd -e js -e jsx -e ts -e tsx -e vue -g "*avatar*" --exec grep -l -i "dicebear" {} \; # Check for breaking changes by examining git history echo "Checking git history for previous dicebear version..." git log -p package.json | grep -A 2 -B 2 "@dicebear"Length of output: 10863
src/components/UserPortal/CreateDirectChat/CreateDirectChat.tsx (1)
27-31: 💡 Codebase verification
⚠️ Potential issueThe return type
ApolloQueryResult<string>is incorrect and should be updatedThe
chatsListRefetchfunction is used to refresh the chat list after creating a chat, but its return type is incorrectly specified. Based on theUSERS_CONNECTION_LISTquery definition, it should return a complex object containing user data withfirstNameandlastNamefields, not a string. Update the return type to match the actual query result structure:Promise<ApolloQueryResult<{ users: { user: { firstName: string; lastName: string; } } }>>🔗 Analysis chain
Review the Apollo query result type.
While removing
anytypes improves type safety, the return typePromise<ApolloQueryResult<string>>seems incorrect. Apollo queries typically return complex objects rather than primitive strings. Consider using the actual query result type or a more appropriate interface.Let's verify the actual query result type:
Let's gather more information about the query definition and its usage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the query definition to understand the expected return type rg -A 5 "USERS_CONNECTION_LIST" --type tsLength of output: 10922
Script:
#!/bin/bash # Get the full query definition and its return type rg -A 15 "export const USERS_CONNECTION_LIST = gql" --type ts # Find where chatsListRefetch is used ast-grep --pattern 'chatsListRefetch($$$)'Length of output: 1163
src/components/UserPortal/CreateDirectChat/CreateDirectChat.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
schema.graphql (3)
208-225: LGTM! Well-structured chat type definitions.The new chat types are well-designed with proper separation between direct and group chats. The types include all necessary fields and maintain consistency in timestamps and IDs.
Consider adding these fields to enhance functionality:
lastMessagefield in both chat types for quick previewisTypingfield for real-time typing indicatorsreadByfield in messages to track message read statusAlso applies to: 464-482
1558-1567: Add input validation for chat creation.The input types are well-structured but lack validation constraints.
Consider adding these validations:
- Minimum/maximum length for group chat title
- Minimum number of users (e.g., at least 2 for direct chat)
- Maximum number of users for group chats
Example:
input createGroupChatInput { organizationId: ID! title: String! @constraint(minLength: 1, maxLength: 100) userIds: [ID!]! @constraint(minItems: 2, maxItems: 100) }
732-739: Add support for file attachments in chat messages.The message sending mutations currently only support text content.
Consider adding file attachment support:
input ChatMessageInput { messageContent: String! fileAttachments: [Upload] } type Mutation { sendMessageToDirectChat( chatId: ID! message: ChatMessageInput! ): DirectChatMessage! }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
schema.graphql(14 hunks)src/components/UserPortal/CreateDirectChat/CreateDirectChat.tsx(1 hunks)
🔇 Additional comments (2)
src/components/UserPortal/CreateDirectChat/CreateDirectChat.tsx (1)
27-30: 🛠️ Refactor suggestion
Verify the Apollo query result type.
While changing id: any to id: string improves type safety, the return type ApolloQueryResult<string> seems incorrect. Apollo queries typically return complex objects rather than primitive types.
Let's verify the actual query response structure:
Consider using a more specific type that matches your GraphQL schema, for example:
Promise<ApolloQueryResult<{
chats: {
id: string;
// other chat fields
}[];
}>>schema.graphql (1)
1162-1164: LGTM! Well-implemented real-time message subscriptions.
The subscription types are properly defined for both direct and group chat messages, enabling real-time updates.
|
I reverted the changes from schema.graphql file . Actually when i was clicking save due to prettier the commas were getting removed. So, this was causing the issue. Do i have to revert changes from other files as well ? because the test is still failing even after reverting changes from schema.graphql. see https://github.com/PalisadoesFoundation/talawa-admin/actions/runs/11721560114/job/32649240787?pr=2412#logs |
|
Please increase the code coverage |
Okay, doing it |
|
I have increased the code coverage . |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/screens/UserPortal/Posts/Posts.tsx (1)
211-228: Consider using object shorthand notation for cleaner code.The data transformation logic is correct and handles edge cases well. However, we can make it more concise using object shorthand notation where property names match variable names.
Consider this refactor for improved readability:
const allLikes: InterfacePostLikes = likedBy.map(({ firstName, lastName, _id }) => ({ firstName, lastName, id: _id, })); const postComments: InterfacePostComments = comments?.map(({ id, creator, likeCount, likedBy, text }) => ({ id, creator: { firstName: creator?.firstName ?? '', lastName: creator?.lastName ?? '', id: creator?.id ?? '', email: creator?.email ?? '', }, likeCount, likedBy: likedBy?.map(like => ({ id: like?.id ?? '' })) ?? [], text, }));
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/screens/UserPortal/Posts/Posts.tsx(2 hunks)
🔇 Additional comments (2)
src/screens/UserPortal/Posts/Posts.tsx (2)
57-64: LGTM: Type definition changes look good.
The standardization of ID field names from _id to id improves consistency in the codebase.
Line range hint 279-281: Verify title alignment fix.
While the code structure looks correct, please ensure that the alignment fix mentioned in PR #2071 is working as expected across different screen sizes.
✅ Verification successful
Let me check the specific CSS module for the Posts component and any related changes.
Based on the CSS changes history and content, I can now provide a final response:
Title alignment has been properly implemented
The code shows that the heading styles have been properly defined with consistent sizing (1.1rem) and the post container has appropriate spacing with proper margins and padding. Recent changes in PR #1896 include a complete redesign of the Posts screen that properly addresses title alignment through:
- Consistent font size for headings
- Proper container spacing with 1rem padding
- Clear margin structure (0.5rem top, 1rem bottom) for the post input container
- Responsive layout with proper width constraints
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for related CSS changes that might affect title alignment
# Look for CSS module changes
rg -l "title|heading" --type css
Length of output: 2544
Script:
#!/bin/bash
# Check the Posts.module.css file content for heading styles
cat src/screens/UserPortal/Posts/Posts.module.css
# Check git history for recent changes to this CSS file
git log -p src/screens/UserPortal/Posts/Posts.module.css
Length of output: 8893
|
Can I raise another PR for issue #2402, as it is assigned to me? I just want to confirm that it won't cause any conflicts with the next PR. |
varshith257
merged commit cd9dd1a
into
PalisadoesFoundation:develop
palisadoes
changed the title
What kind of change does this PR introduce?
bugfix
Issue Number:
Fixes #2071
Did you add tests for your changes?
No
Snapshots/Videos:
Before
After
If relevant, did you update the documentation?
Summary
I updated the code such that the "Posts" title aligns properly.
Does this PR introduce a breaking change?
No
Other information
Have you read the contributing guide?
Yes
Summary by CodeRabbit
Release Notes
New Features
OrganizationCardandUserScreencomponents.Bug Fixes
UserScreencomponent.Documentation
UserScreentests.Chores