Fixed error message in PostCard.tsx #3152
Conversation
WalkthroughThe pull request enhances the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/components/UserPortal/PostCard/PostCard.tsx (1)
228-232: Consider storing or logging the modified error message.You are currently calling
error.message.replace(...).replace(...).trim()without assigning the result to a variable or using it elsewhere. If your intent is to log or store the cleaned-up error message (e.g., for debugging or analytics), make sure to persist it or log it in the console for developers’ reference. Otherwise, this line is effectively a no-op.if (error instanceof Error) { - error.message - .replace(/^Comment validation failed:\s*/, '') - .replace(/Path `text` is required\./, '') - .trim(); toast.error('Please enter a comment before submitting.'); } else { toast.error('An unexpected error occurred. Please try again.'); errorHandler(t, error); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/UserPortal/PostCard/PostCard.tsx(1 hunks)
🔇 Additional comments (1)
src/components/UserPortal/PostCard/PostCard.tsx (1)
233-233: Good user-friendly message.Replacing the technical validation failure with “Please enter a comment before submitting.” provides a much better user experience.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #3152 +/- ##
=====================================================
+ Coverage 26.46% 89.83% +63.37%
=====================================================
Files 300 331 +31
Lines 7568 8582 +1014
Branches 1652 1901 +249
=====================================================
+ Hits 2003 7710 +5707
+ Misses 5434 611 -4823
- Partials 131 261 +130
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Please add tests as shown here. Click the CodeCov links above to see for yourself
- https://app.codecov.io/gh/PalisadoesFoundation/talawa-admin/pull/3152?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=PalisadoesFoundation
- Please add translations for the text. It's only in English.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/UserPortal/PostCard/PostCard.spec.tsx (1)
665-665: Remove console.log statementDevelopment logging statement should be removed before merging.
- console.log('running empty comment test case');public/locales/fr/translation.json (1)
1053-1053: Consider refining the French translation.The translation is grammatically correct but could be more natural. Consider changing:
- "comment.notFound": "Veuillez entrer ce commentaire avant de soumettre" + "comment.notFound": "Veuillez entrer un commentaire avant de soumettre"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
public/locales/en/translation.json(1 hunks)public/locales/fr/translation.json(1 hunks)public/locales/hi/translation.json(1 hunks)public/locales/sp/translation.json(1 hunks)public/locales/zh/translation.json(1 hunks)src/components/UserPortal/PostCard/PostCard.spec.tsx(2 hunks)src/components/UserPortal/PostCard/PostCard.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/UserPortal/PostCard/PostCard.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test Application
🔇 Additional comments (5)
src/components/UserPortal/PostCard/PostCard.spec.tsx (1)
664-713: LGTM! Well structured test caseThe test case thoroughly validates the empty comment error handling:
- Proper component setup with required props
- Correct simulation of user interactions
- Appropriate assertion of error message using i18n
public/locales/zh/translation.json (1)
1053-1053: LGTM! Clear and consistent translationThe Chinese translation maintains the meaning while using natural language phrasing.
public/locales/en/translation.json (1)
1093-1093: LGTM! Clear error messageThe English message is user-friendly and clearly explains what action is needed.
public/locales/hi/translation.json (1)
1053-1053: LGTM! Accurate Hindi translationThe Hindi translation accurately conveys the meaning while maintaining natural language flow.
public/locales/sp/translation.json (1)
1054-1054: LGTM! Spanish translation is accurate and natural.The translation effectively conveys the message while maintaining proper Spanish grammar and natural phrasing.
|
Your tests are failing. Please don't assign reviewers if they are. Please be considerate of our time. |
|
@rishav-jha-mech Thanks for reviewing. I'll make the necessary changes. |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/components/UserPortal/PostCard/PostCard.tsx (1)
Add test coverage for empty and whitespace-only comment validation
The current test file
src/components/UserPortal/PostCard/PostCard.spec.tsxlacks coverage for the new validation logic. Please add test cases for:
- Submitting an empty comment
- Submitting a whitespace-only comment
- Verification of correct error messages using translation keys
🔗 Analysis chain
Line range hint
199-246: Add test coverage for the new validation logic.As requested in previous reviews, please add tests for:
- Empty comment validation
- Whitespace-only comment validation
- Error message translations
Here's a verification script to check current test coverage:
Would you like me to provide example test cases for these scenarios?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check test coverage for comment validation # Look for existing test cases echo "Searching for existing test cases..." rg -l "test.*empty.*comment|test.*whitespace.*comment" --type ts # Check translation keys echo "Checking translation files for error messages..." fd "translation.json" | xargs rg "emptyCommentError|unexpectedError"Length of output: 892
Script:
#!/bin/bash # Check the content of the test file for specific test cases echo "Examining test file content..." rg "describe|it\(.*comment" -A 5 src/components/UserPortal/PostCard/PostCard.spec.tsx # Look for toast/error handling in tests echo -e "\nChecking error handling in tests..." rg "toast\.(error|success)" src/components/UserPortal/PostCard/PostCard.spec.tsxLength of output: 1156
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/UserPortal/PostCard/PostCard.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test Application
- GitHub Check: Analyse Code With CodeQL (javascript)
🔇 Additional comments (1)
src/components/UserPortal/PostCard/PostCard.tsx (1)
199-204: LGTM! Well-implemented empty comment validation.The validation is placed correctly before the API call, and it properly handles whitespace-only comments. Good use of translations for the error message.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/UserPortal/PostCard/PostCard.tsx (1)
234-244: Improve error handling structure.While the error handling is functional, it could be more robust:
- Console errors should include more context
- The error type check could be more specific
Consider this improvement:
- // Handle errors - // Log the original error for debugging - console.error(error); + // Log error with context for debugging + console.error('Error creating comment:', error); - // Show user-friendly translated message - toast.error( - error instanceof Error && - error.message.includes('Comment validation failed') - ? t('emptyCommentError') - : t('unexpectedError'), - ); + // Show user-friendly translated message based on error type + if (error instanceof Error) { + const isValidationError = error.message.includes('Comment validation failed'); + toast.error(isValidationError ? t('emptyCommentError') : t('unexpectedError')); + } else { + toast.error(t('unexpectedError')); + }src/components/UserPortal/PostCard/PostCard.spec.tsx (1)
664-717: Add more test coverage for error scenarios.While the empty comment validation test is good, consider adding tests for:
- Network errors
- Server validation errors
- Successful comment submission after a failed attempt
Here's a suggested test case for network errors:
test('Comment submission displays error toast when network error occurs', async () => { const errorLink = new StaticMockLink([{ request: { query: CREATE_COMMENT_POST, variables: { postId: '1', comment: 'test comment' }, }, error: new Error('Network error'), }]); render( <MockedProvider link={errorLink}> <PostCard {...cardProps} /> </MockedProvider> ); userEvent.click(screen.getByTestId('viewPostBtn')); userEvent.type(screen.getByTestId('commentInput'), 'test comment'); userEvent.click(screen.getByTestId('createCommentBtn')); await waitFor(() => { expect(toast.error).toHaveBeenCalledWith( i18nForTest.t('postCard.unexpectedError') ); }); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
public/locales/en/translation.json(1 hunks)public/locales/fr/translation.json(1 hunks)public/locales/hi/translation.json(1 hunks)public/locales/sp/translation.json(1 hunks)public/locales/zh/translation.json(1 hunks)src/components/UserPortal/PostCard/PostCard.spec.tsx(2 hunks)src/components/UserPortal/PostCard/PostCard.tsx(2 hunks)
🔇 Additional comments (6)
src/components/UserPortal/PostCard/PostCard.tsx (1)
199-203: LGTM! Good validation for empty comments.The empty comment validation is well-implemented with proper whitespace handling and user-friendly error messages using translations.
public/locales/zh/translation.json (1)
1055-1057: LGTM! Well-structured translations.The Chinese translations are clear and maintain consistent meaning with the English version.
public/locales/en/translation.json (1)
1095-1097: LGTM! Clear and consistent error messages.The English translations are user-friendly and properly punctuated.
public/locales/hi/translation.json (1)
1055-1057: LGTM! The Hindi translations are clear and accurate.The new error messages are well-translated and maintain consistency with other localization files while being user-friendly.
public/locales/fr/translation.json (1)
1055-1057: LGTM! The French translations are well-crafted.The error messages are properly translated with correct grammar and maintain a professional tone.
public/locales/sp/translation.json (1)
1056-1058: LGTM! The Spanish translations are accurate and consistent.The error messages are properly translated using formal Spanish, maintaining clarity and consistency with other language versions.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/components/UserPortal/PostCard/PostCard.spec.tsx (2)
665-665: Remove debug console.log statement.Debug logging should be removed before committing the code.
- console.log('Starting empty comment validation test');
694-694: Remove redundant toast.error check.This check is unnecessary as the toast mock is already defined in the test setup at line 47.
- expect(toast.error).toBeDefined();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/UserPortal/PostCard/PostCard.spec.tsx(2 hunks)src/components/UserPortal/PostCard/PostCard.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/UserPortal/PostCard/PostCard.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test Application
🔇 Additional comments (4)
src/components/UserPortal/PostCard/PostCard.spec.tsx (4)
3-3: LGTM: Import statement updated correctly.The
waitForutility from@testing-library/reactis correctly imported to handle asynchronous assertions in the new test cases.
664-717: LGTM: Empty comment validation test is well-structured.The test case properly validates the empty comment error scenario by:
- Setting up the test environment with required props
- Simulating user actions (opening post view, typing empty comment, clicking submit)
- Verifying the correct error message is displayed using i18n translation
719-775: LGTM: Network error test is well-implemented.The test case properly validates the network error scenario by:
- Setting up a mock that simulates a network error
- Simulating user actions (opening post view, typing comment, clicking submit)
- Verifying the correct error message is displayed using i18n translation
664-775: Great job on improving test coverage!The new test cases effectively cover the error scenarios mentioned in the PR objectives:
- Empty comment validation
- Network error handling
This addresses the previous review comment about testing error messages and follows testing best practices.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/components/UserPortal/PostCard/PostCard.spec.tsx (3)
665-665: Remove debug console.log statement.Debug logging should be removed before committing the code.
- console.log('Starting empty comment validation test');
664-717: Consider expanding empty comment validation test cases.The test could be more comprehensive by:
- Testing with whitespace-only comments
- Testing with null/undefined values
- Using test.each for multiple test cases
Example implementation:
test.each([ ['', 'empty string'], [' ', 'whitespace only'], [null, 'null value'], [undefined, 'undefined value'] ])('shows error for invalid comment: %s (%s)', async (input, _desc) => { // ... test setup ... userEvent.type(screen.getByTestId('commentInput'), input || ''); userEvent.click(screen.getByTestId('createCommentBtn')); await waitFor(() => { expect(toast.error).toHaveBeenCalledWith( i18nForTest.t('postCard.emptyCommentError'), ); }); });
719-781: Refactor test setup to reduce duplication.The test setup contains duplicated cardProps. Consider extracting common test props into a shared fixture.
Example implementation:
// At the top of the file const baseCardProps = { id: '1', userImage: 'image.png', creator: { firstName: 'test', lastName: 'user', email: '[email protected]', id: '1', }, // ... other common props }; // In the test test('Comment submission displays error toast when network error occurs', async () => { const cardProps = { ...baseCardProps, // Add/override specific props for this test }; // ... rest of the test });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/UserPortal/PostCard/PostCard.spec.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test Application
🔇 Additional comments (3)
src/components/UserPortal/PostCard/PostCard.spec.tsx (3)
3-3: LGTM!The addition of
waitForimport is appropriate for the new async assertions.
747-758: LGTM! Good error simulation setup.The error simulation using StaticMockLink is well implemented and follows best practices for testing GraphQL mutations.
664-781: Well-implemented test coverage for error scenarios!The new tests effectively cover empty comment validation and network error handling, addressing the previous review feedback. The implementation is solid and follows testing best practices.
|
@rishav-jha-mech Could you please review my commit? All of my test cases were passing initially, but after Code Rabbit suggested some changes, I added additional test cases, including edge cases. However, Code Rabbit is still not approving it. |
| @@ -661,6 +661,125 @@ describe('Testing PostCard Component [User Portal]', () => { | |||
| await wait(); | |||
| }); | |||
|
|
|||
| test('Comment validation displays an error toast when an empty comment is submitted', async () => { | |||
| console.log('Starting empty comment validation test'); | |||
There was a problem hiding this comment.
@Nivedita-Chhokar code rabbit is asking you to remove this line
console.log should not be here
There was a problem hiding this comment.
@rishav-jha-mech Thank you for your review. I'll remove that line, but could you share your thoughts on the other suggestions made by Code Rabbit? Also, if you have any additional recommendations, I'd appreciate them.
palisadoes
merged commit 39d202f
into
PalisadoesFoundation:develop-postgres
What kind of change does this PR introduce?
Bugfix
Issue Number:
Fixes #3142
Did you add tests for your changes?
No
Snapshots/Videos:
Before :
After :
Untitled.video.-.Made.with.Clipchamp.mp4
If relevant, did you update the documentation?
No
Summary
This PR resolves the issue where submitting an empty comment displayed a technical error message (Comment validation failed: text: Path 'text' is required.). The error message is now replaced with a user-friendly one (Please enter a comment before submitting.), enhancing the user experience. The technical message remains in the logs for developers.
Does this PR introduce a breaking change?
No
Other information
None
Have you read the contributing guide?
Yes
Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
New Features
Tests