Updated User and Admin portal with file uploads (GSoC 24) #2408
Conversation
…nt in post and user feeds - Added edit post functionality for user feed - Implemented media preview for user post creation - Updated delete post feature for users - Enhanced admin post feed with post creation, update, and delete options - Integrated new APIs for file upload and management across user and admin feeds
…into feature/feed-file-upload
…into feature/feed-file-upload
WalkthroughThis pull request includes several significant changes across multiple files. Key modifications involve the removal of the Changes
Assessment against linked issues
Possibly related PRs
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
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| </> | ||
| {previewUrl && ( | ||
| <div className={styles.preview}> | ||
| <img src={previewUrl} alt={t('postImage')} /> |
Check warning
Code scanning / CodeQL
DOM text reinterpreted as HTML Medium
| <video controls> | ||
| <source src={postformState.postvideo} type="video/mp4" /> | ||
| <video controls data-testid="videoPreview"> | ||
| <source src={previewUrl} type="video/mp4" /> |
Check warning
Code scanning / CodeQL
DOM text reinterpreted as HTML Medium
| {mediaPreviewUrl ? ( | ||
| editedMediaType === 'image' ? ( | ||
| <img | ||
| src={mediaPreviewUrl} |
Check warning
Code scanning / CodeQL
DOM text reinterpreted as HTML Medium
| /> | ||
| ) : ( | ||
| <video | ||
| src={mediaPreviewUrl} |
Check warning
Code scanning / CodeQL
DOM text reinterpreted as HTML Medium
| ) : ( | ||
| <video controls data-testid="videoPreview"> | ||
| <source | ||
| src={previewUrl} |
Check warning
Code scanning / CodeQL
DOM text reinterpreted as HTML Medium
| <img | ||
| src={postformState.addMedia} | ||
| src={previewUrl} |
Check warning
Code scanning / CodeQL
DOM text reinterpreted as HTML Medium
| <source src={postformState.addMedia} type={file.type} />( | ||
| {t('tag')}) | ||
| <source | ||
| src={previewUrl} |
Check warning
Code scanning / CodeQL
DOM text reinterpreted as HTML Medium
chandel-aman
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 39
🧹 Outside diff range and nitpick comments (53)
.husky/pre-commit (1)
Line range hint
4-11: Consider improving the pre-commit hook's robustness.The script could benefit from some improvements:
- Error handling to prevent commits if any command fails
- More selective git staging to avoid accidentally staging unrelated changes
Consider applying these improvements:
#!/usr/bin/env sh . "$(dirname -- "$0")/_/husky.sh" +# Exit on error +set -e + # npm run format:fix npm run lint:fix npm run lint-staged npm run typecheck npm run update:toc -git add . +# Only stage files that were modified by the hooks +git diff --name-only | xargs git addsrc/components/UserPortal/CommentCard/CommentCard.module.css (1)
17-22: Consider consolidating duplicate image styling.There appears to be some style duplication between
.creator imgand.personImageclasses, both handling circular images. Consider consolidating these styles into a shared class for better maintainability.+.circularImage { + border-radius: 50%; +} + .creator img { width: 2rem; height: 2rem; - border-radius: 100%; + composes: circularImage; object-fit: cover; } .personImage { - border-radius: 50%; + composes: circularImage; margin-right: 20px; }Also applies to: 24-27
src/components/UserPortal/StartPostModal/StartPostModal.module.css (1)
21-29: Consider enhancing preview container for multiple file support.The current layout using
justify-content: centermight not handle multiple file previews optimally. Consider using a grid or flex-wrap layout for better multi-file preview support..preview { display: flex; + flex-wrap: wrap; + gap: 10px; position: relative; width: 100%; margin-top: 10px; - justify-content: center; + justify-content: flex-start; overflow: hidden; border-radius: 1rem; }src/components/UserPortal/PromotedPost/PromotedPost.tsx (3)
Line range hint
6-10: Update interface to align with new file handling structureThe interface should be updated to match the new file handling approach mentioned in the PR objectives and summary. The current simple
imagestring property doesn't align with the new media file structure being implemented across the application.Consider updating the interface:
interface InterfacePostCardProps { id: string; - image: string; + mediaFile?: { + url: string; + fileType: string; + fileName: string; + }; title: string; }
Line range hint
41-43: Add alt text for accessibilityThe image element lacks an alt attribute, which is essential for accessibility.
Apply this change:
- <img src={props.image} className={styles.imageContainer} /> + <img + src={props.image} + className={styles.imageContainer} + alt={`Promoted content: ${props.title}`} + />
Line range hint
36-39: Remove duplicate content displayThe title is currently displayed twice - in both Card.Title and Card.Text. This seems redundant and might be confusing to users.
Consider either:
- Removing the duplicate Card.Text, or
- Adding a separate description field to the interface for meaningful content in Card.Text
src/screens/UserPortal/Posts/Posts.module.css (1)
130-130: Consider documenting the style organization.Since core styles have been moved out of this module, it would be helpful to add a comment indicating where these styles are now maintained (e.g., shared components, design system, etc.) to help other developers understand the styling structure.
Add a comment like:
+/* Core button styles moved to shared/components/Button.css */ .startPostBtn { border-radius: 2rem; }src/components/UserPortal/PostCard/PostCard.module.css (1)
182-188: Consider making media width responsiveWhile combining image and video styles reduces duplication, the fixed width of 20rem might cause issues on smaller screens.
Consider using this more responsive approach:
.postImage, .postVideo { - width: 20rem; + width: 100%; + max-width: 20rem; aspect-ratio: 1.5; object-fit: cover; border-radius: 0; }src/GraphQl/Queries/OrganizationQueries.ts (1)
30-45: Well-structured file object design!The new file object structure is comprehensive and well-designed for robust media handling. It includes essential metadata, security features (hash), and proper MinIO integration (objectKey).
Consider these operational recommendations:
- Ensure proper indexing on
_idandhash.valuefields for efficient file deduplication.- Implement caching for frequently accessed files using the
hash.valueas cache key.- Consider implementing a cleanup job for files with
statusindicating temporary or failed uploads.src/components/UserPortal/StartPostModal/StartPostModal.test.tsx (3)
24-26: Consider using a more type-safe fetch mock.The current fetch mock could benefit from TypeScript types to ensure type safety across test cases.
const mockFetch = jest.fn() as jest.MockedFunction<typeof global.fetch>; global.fetch = mockFetch;
33-63: Consider moving mock data to a separate test utils file.The
mockUserDataandmockPropsobjects could be moved to a separate test utils file (e.g.,StartPostModal.test.utils.ts) to improve maintainability and reusability across related test files.
79-296: Improve test organization with describe blocks.Consider grouping related tests using describe blocks for better organization and readability. For example:
describe('StartPostModal Component', () => { // ... beforeEach setup ... describe('Post Creation', () => { // Tests related to creating posts it('should handle successful post creation', ...); it('should handle post creation error', ...); it('should use default error message when no specific error is returned', ...); }); describe('Media Handling', () => { // Tests related to media upload and preview it('should display image preview when an image file is selected', ...); it('should display video preview when a video file is selected', ...); it('should remove preview when the selected file is removed', ...); }); describe('UI Interactions', () => { // Tests related to UI state and interactions it('should disable post button when post body is empty', ...); it('should consider whitespace-only content as empty', ...); it('should trigger file input click when Add Media button is clicked', ...); }); });🧰 Tools
🪛 Biome
[error] 222-225: Disallow duplicate setup and teardown hooks.
Disallow beforeEach duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
src/screens/UserPortal/Posts/Posts.test.tsx (4)
30-153: LGTM! Consider adding TypeScript interfaces for mock data.The mock data structures are well-organized and comprehensive, properly representing the file upload capabilities. They include all necessary fields for both posts and advertisements.
Consider defining TypeScript interfaces for these mock data structures to improve type safety and maintainability:
interface MockFile { _id: string; fileName: string; mimeType: string; size: number; hash: { value: string; algorithm: string; }; uri: string; metadata: { objectKey: string; }; visibility: string; status: string; } interface MockPost { _id: string; title: string; text: string; file: MockFile | null; // ... other fields }
251-259: Consider extracting timeout values to constants.The timeout value of 2000ms is used multiple times throughout the tests. Consider extracting it to a constant for better maintainability.
const DEFAULT_TIMEOUT = 2000; await waitFor( () => { expect( screen.getByText(/Start a post, with text or media/i), ).toBeInTheDocument(); }, { timeout: DEFAULT_TIMEOUT }, );
Line range hint
219-226: Consider usingwaitfunction consistently.The test file has both
waitandwaitForfunctions. Consider standardizing onwaitForas it provides better error messages and is the recommended approach in React Testing Library.
408-426: LGTM! Consider adding more edge cases.The redirect test is good, but consider adding tests for:
- Invalid organization IDs
- Permission-related redirects
- Session timeout scenarios
public/locales/en/translation.json (4)
688-690: Fix spelling in error message keys.The error messages look good, but there's a spelling inconsistency in the key names.
Apply this diff to maintain consistent naming:
- "postCreationFailed":"Failed to create the post." + "postCreationFailed":"Post creation failed."
730-733: Consider consolidating duplicate error messages.The error messages
noTitleAndDescriptionandpostCreationFailedare duplicated from theorgPostsection. Consider using a common section for shared error messages.Consider moving these messages to a common
errorsorcommonsection to promote reuse and easier maintenance.
1048-1052: Fix spelling in success message.There's a spelling error in the success message key.
Apply this diff to fix the spelling:
- "deletionSuccessfull": "Successfully deleted the post.", + "deletionSuccessful": "Successfully deleted the post."Also, consider using consistent message formats. Some messages start with "Successfully" while others use "Failed to".
1070-1072: Ensure consistent error message structure.The error messages in this section should follow the same pattern as other sections for consistency.
Consider applying this diff to maintain consistency:
- "noTitleAndDescription": "Can not create a post with an empty body.", + "noTitleAndDescription": "Title and description cannot be empty.",public/locales/fr/translation.json (1)
1048-1052: Consider improving the date placeholder translation.The translations are accurate, but the date placeholder could be more explicit for better localization.
Consider updating line 1048:
- "postedOn": "Publié le {{date}}", + "postedOn": "Publié le {{date, DD/MM/YYYY}}",This makes the expected date format more explicit for French localization.
src/components/UserPortal/StartPostModal/StartPostModal.tsx (1)
201-201: Allow posting media without text contentCurrently, the "Post" button is disabled when the text input is empty, even if a media file is selected. Consider allowing users to post media without requiring text input.
Apply this diff:
- disabled={!postformState.postinfo.trim()} + disabled={!postformState.postinfo.trim() && !postformState.mediaFile}src/screens/UserPortal/Posts/Posts.tsx (7)
Line range hint
62-62: Specify Type for 'posts' State VariableConsider specifying the type for the
postsstate variable inuseStateto enhance type safety and prevent potential runtime errors.Apply this diff to specify the type:
-const [posts, setPosts] = useState([]); +const [posts, setPosts] = useState<{ node: InterfacePostNode }[]>([]);
Line range hint
63-63: Specify Type for 'pinnedPosts' State VariableSimilarly, specify the type for the
pinnedPostsstate variable to maintain consistency and improve type safety.Apply this diff:
-const [pinnedPosts, setPinnedPosts] = useState([]); +const [pinnedPosts, setPinnedPosts] = useState<{ node: InterfacePostNode }[]>([]);
212-212: Remove Debugging Statement 'console.log'The
console.logstatement should be removed to prevent unnecessary logging in production environments.Apply this diff:
- console.log(promotedPostsData.organizations[0].advertisements);
333-336: Internationalize Button LabelThe button label "Start a post, with text or media" is hardcoded. For consistency and to support multiple languages, consider using the translation function
t().Apply this diff:
- Start a post, with text or media + {t('startPostWithMedia')}Also, add the key
startPostWithMediato your translation files.
345-350: Move Inline Styles to CSS ModuleThe inline styles applied to the
divcan be moved to the CSS module to enhance readability and maintainability.In your
Posts.module.css, add:.carouselContainer { display: flex; justify-content: space-between; align-items: center; margin-top: 1rem; }Then, apply the class:
- <div - style={{ - justifyContent: `space-between`, - alignItems: `center`, - marginTop: `1rem`, - }} - > + <div className={styles.carouselContainer}>
Line range hint
98-100: Ensure Safe Access to Nested Data in 'useEffect'When accessing
data.organizations[0].posts.edges, there is a risk of encounteringTypeErrorif any property isundefined. Consider adding additional checks or using optional chaining.Apply this diff:
- if (data) { + if (data?.organizations?.[0]?.posts?.edges) {Alternatively, handle the case where
organizationsarray might be empty.if (data?.organizations?.length) { setPosts(data.organizations[0].posts.edges); }
Line range hint
157-159: Validate 'userId' Before Using in Query
userIdmay benull, which can cause unexpected behavior in theuseQueryhook forUSER_DETAILS. EnsureuserIdis valid before making the query.Apply this diff:
-const { data: userData } = useQuery(USER_DETAILS, { - variables: { id: userId }, -}); +const { data: userData } = useQuery(USER_DETAILS, { + variables: { id: userId }, + skip: !userId, +});This way, the query will be skipped if
userIdisnull.src/utils/interfaces.ts (2)
186-201: Define a dedicated interface for thefileobjectThe
fileobject withinInterfaceQueryOrganizationPostListItemis complex. Defining a separate interface enhances readability and maintainability.Suggested refactor:
+interface InterfaceFile { + _id: string; + fileName: string; + mimeType: string; + size: number; + hash: { + value: string; + algorithm: string; + }; + uri: string; + metadata: { + objectKey: string; + }; + visibility: FileVisibility; + status: FileStatus; +} export interface InterfaceQueryOrganizationPostListItem { posts: { edges: { node: { _id: string; title: string; text: string; - file: { - // file properties - }; + file: InterfaceFile; creator: { _id: string; firstName: string; lastName: string; email: string; }; // other properties }; cursor: string; }[]; pageInfo: { // page info properties }; totalCount: number; }; }
199-200: Use enums forvisibilityandstatuspropertiesUsing string literals for
visibilityandstatuscan lead to typos and inconsistent values. Defining enums improves type safety and code clarity.Define enums:
enum FileVisibility { PUBLIC = 'PUBLIC', PRIVATE = 'PRIVATE', // add other visibility options if necessary } enum FileStatus { ACTIVE = 'ACTIVE', INACTIVE = 'INACTIVE', // add other status options if necessary }Update the
InterfaceFile:interface InterfaceFile { _id: string; fileName: string; mimeType: string; size: number; hash: { value: string; algorithm: string; }; uri: string; metadata: { objectKey: string; }; - visibility: string; - status: string; + visibility: FileVisibility; + status: FileStatus; }src/components/OrgPostCard/OrgPostCard.tsx (2)
237-237: Remove console.log statement from production codeThe
console.log(data);statement may expose sensitive information in the console. Consider removing it to avoid potential security risks.Apply this diff:
const data = await response.json(); - console.log(data); if (!response.ok) {
264-266: Revoke object URLs to prevent memory leaksWhen using
URL.createObjectURL, it's important to revoke the object URL when it's no longer needed to prevent memory leaks. Consider revoking the previous URL before setting a new one.Here's how you could modify the code:
if (file) { + if (previewUrl) { + URL.revokeObjectURL(previewUrl); + } const newPreviewUrl = URL.createObjectURL(file); setPreviewUrl(newPreviewUrl); }Also, ensure to revoke the object URL when the component unmounts.
src/screens/OrgPost/OrgPost.tsx (2)
138-139: Remove commented-out code to maintain code cleanlinessThe commented-out code for
useMutationis no longer necessary due to the transition to using REST APIs. Keeping such code can clutter the codebase and cause confusion.Apply this diff to remove the unused code:
- // const [create, { loading: createPostLoading }] = - // useMutation(CREATE_POST_MUTATION);
248-250: Revoke object URLs immediately after usage to free up memoryWhile you have a cleanup function in the
useEffect, it's good practice to revoke the object URL right after the file is no longer needed to prevent memory leaks.Consider updating the
clearMediafunction to revoke the URL:const clearMedia = (): void => { setPostFormState((prev) => ({ ...prev, mediaFile: null })); if (previewUrl) { + URL.revokeObjectURL(previewUrl); setPreviewUrl(null); } const fileInput = document.getElementById('mediaFile') as HTMLInputElement; if (fileInput) { fileInput.value = ''; } };Also applies to: 221-226
src/components/UserPortal/PostCard/PostCard.tsx (5)
48-48: Ensure optional handling of the new 'image' property in 'creator' objectThe addition of the
imageproperty to thecreatorobject inInterfaceCommentCardPropsis appropriate. Please ensure that all components consuming this interface, such asCommentCard, can handle cases wherecreator.imagemay benullorundefinedto prevent potential runtime errors.
289-289: Correct typo in success messageThere is a typo in the toast success message. It should be "deletionSuccessful" instead of "deletionSuccessfull".
Apply this diff to fix the typo:
- toast.success(t('deletionSuccessfull')); + toast.success(t('deletionSuccessful'));
306-306: Update error message for unsupported media typesWhen an unsupported file type is selected, the error message "noMediaSelected" might confuse users. Consider updating it to inform users that the selected file type is not supported.
Apply this diff to improve user feedback:
- toast.error(t('noMediaSelected')); + toast.error(t('unsupportedMediaType'));Ensure that the translation key
unsupportedMediaTypeis added to your localization files with an appropriate message.
311-315: Revoke object URLs to prevent memory leaksWhen using
URL.createObjectURL, it's important to revoke the object URL after usage to prevent memory leaks. Consider revoking the URL when the component unmounts or when the media is cleared.You can modify your code as follows:
const previewUrl = URL.createObjectURL(file); setMediaPreviewUrl(previewUrl); + // Clean up the object URL when it's no longer needed + useEffect(() => { + return () => { + if (mediaPreviewUrl) { + URL.revokeObjectURL(mediaPreviewUrl); + } + }; + }, [mediaPreviewUrl]);Alternatively, revoke the URL in the
handleClearMediafunction before settingmediaPreviewUrltonull.
317-324: Revoke object URL inhandleClearMediaIn the
handleClearMediafunction, ensure that you revoke the object URL to free up memory resources.Update the function as follows:
const handleClearMedia = () => { + if (mediaPreviewUrl) { + URL.revokeObjectURL(mediaPreviewUrl); + } setEditedMedia(null); setEditedMediaType(null); setMediaPreviewUrl(null); if (fileInputRef.current) { fileInputRef.current.value = ''; } };src/components/UserPortal/PostCard/PostCard.test.tsx (5)
666-668: Simplify condition using optional chainingYou can simplify the condition by using optional chaining for better readability.
Apply this diff to simplify the code:
-if (global.URL && global.URL.createObjectURL) { +if (global.URL?.createObjectURL) { global.URL.createObjectURL = jest.fn(); }🧰 Tools
🪛 Biome
[error] 666-666: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
753-753: Simplify redundant logical expressionAt line 753, the expression
'Update failed' || 'Failed to update the post'is redundant because the logical OR will always return the first truthy value.Apply this diff to simplify the expression:
-json: () => Promise.resolve({ error: 'Update failed' || 'Failed to update the post'}), +json: () => Promise.resolve({ error: 'Update failed' }),
782-785: Default error handling improvementIn the failed update test case without an error message (lines 782-785), consider providing a default error message to handle cases where the response doesn't include one.
Apply this diff to improve error handling:
- expect(toast.error).toHaveBeenCalledWith('Update failed'); + expect(toast.error).toHaveBeenCalledWith('Update failed' || 'An unexpected error occurred');Ensure that your component displays a generic error message if none is provided by the API.
882-882: Typo in error messageThere's a typo in the error message: "Please select and image or video file." It should be "Please select an image or video file."
Apply this diff to correct the typo:
-expect(toast.error).toHaveBeenCalledWith('Please select and image or video file.'); +expect(toast.error).toHaveBeenCalledWith('Please select an image or video file.');Ensure the same correction is made in the component code where this message originates.
902-904: Simplify condition using optional chainingSimilar to the previous instance, you can use optional chaining for cleaner code.
Apply this diff:
-if (global.URL && global.URL.createObjectURL) { +if (global.URL?.createObjectURL) { global.URL.createObjectURL = jest.fn(); }🧰 Tools
🪛 Biome
[error] 902-902: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/screens/OrgPost/OrgPost.test.tsx (5)
1049-1051: UseuserEvent.uploadinstead offireEvent.changefor file uploadsTo simulate user interactions more accurately when uploading files, consider using
userEvent.uploadinstead offireEvent.change. This aligns with best practices for testing user interactions.Apply this diff to the affected lines:
- fireEvent.change(fileInput, { target: { files: [file] } }); + await userEvent.upload(fileInput, file);Also applies to: 1126-1128, 1175-1177
568-577: Avoid redeclaringrenderWithProvidersinside thedescribeblockDefining
renderWithProvidersagain inside thedescribeblock (lines 568-577) is unnecessary since it's already defined globally. Remove the inner declaration to prevent confusion.Apply this diff:
- // Remove the redeclared function inside the describe block - const renderWithProviders = () => { - return render( - <MockedProvider mocks={generateMocks()} addTypename={false}> - <BrowserRouter> - <I18nextProvider i18n={i18nForTest}> - <OrgPost /> - </I18nextProvider> - </BrowserRouter> - </MockedProvider>, - ); - };
591-806: Optimize the use ofbeforeEachfor common setupThe
beforeEachblock (lines 579-589) sets up mocks forglobal.fetchandglobal.URLand is duplicated before some test suites. Consider moving this setup to a higher scope to avoid repetition.
591-806: Avoid directly modifying global objectsDirectly assigning to
global.fetchandglobal.URLin tests can lead to side effects. Consider using libraries likejest-fetch-mockto mockfetchor utilizejest.spyOnfor more controlled mocking.
68-120: Keep mock data consistent and DRYThe mock data for posts is defined multiple times with similar structures (lines 68-120 and lines 222-326). Consider refactoring the mock data to a separate module or utility function to avoid duplication and ensure consistency.
Also applies to: 222-326
src/components/OrgPostCard/OrgPostCard.test.tsx (3)
21-21: Remove commented-out import statementThe import of
convertToBase64is commented out at line 21 but re-imported at line 24. Consider removing the commented-out import to clean up the code.Apply this diff to remove the commented-out import:
- // import convertToBase64 from 'utils/convertToBase64';Also applies to: 24-24
105-115: Consolidate props definitions to reduce duplicationMultiple props objects (
props,props2,mockProps) with similar structures are defined throughout the tests. Consider creating a single base props object and extending it as needed to reduce code duplication and improve maintainability.Example:
const baseProps = { id: '12', postID: '123', postTitle: 'Event Info', postInfo: 'Time change', postAuthor: 'John Doe', postPhoto: 'test.png', postVideo: 'test.mp4', pinned: false, refetch: jest.fn(), }; const mockProps = { ...baseProps, postTitle: 'Test Post', postInfo: 'Test Info', postAuthor: 'Test Author', postPhoto: 'test.jpg', postVideo: null }; const props2 = { ...baseProps, id: '', pinned: true };Also applies to: 148-148, 272-272, 324-324, 403-403
993-1007: Handle API responses and errors consistentlyIn the tests for form submissions, ensure that API responses and errors are handled consistently. Mock the
fetchAPI to return appropriate responses and verify that the component behaves as expected in success and failure scenarios.Also applies to: 1031-1039, 1062-1071, 1009-1041
📜 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 (26)
.env.example(0 hunks).husky/pre-commit(1 hunks)public/locales/en/translation.json(4 hunks)public/locales/fr/translation.json(4 hunks)public/locales/hi/translation.json(4 hunks)public/locales/sp/translation.json(4 hunks)public/locales/zh/translation.json(4 hunks)src/GraphQl/Queries/OrganizationQueries.ts(2 hunks)src/components/OrgPostCard/OrgPostCard.test.tsx(15 hunks)src/components/OrgPostCard/OrgPostCard.tsx(17 hunks)src/components/UserPortal/CommentCard/CommentCard.module.css(1 hunks)src/components/UserPortal/CommentCard/CommentCard.tsx(2 hunks)src/components/UserPortal/PostCard/PostCard.module.css(3 hunks)src/components/UserPortal/PostCard/PostCard.test.tsx(23 hunks)src/components/UserPortal/PostCard/PostCard.tsx(13 hunks)src/components/UserPortal/PromotedPost/PromotedPost.tsx(1 hunks)src/components/UserPortal/StartPostModal/StartPostModal.module.css(1 hunks)src/components/UserPortal/StartPostModal/StartPostModal.test.tsx(1 hunks)src/components/UserPortal/StartPostModal/StartPostModal.tsx(3 hunks)src/screens/OrgPost/OrgPost.test.tsx(2 hunks)src/screens/OrgPost/OrgPost.tsx(14 hunks)src/screens/OrganizationDashboard/OrganizationDashboardMocks.ts(1 hunks)src/screens/UserPortal/Posts/Posts.module.css(1 hunks)src/screens/UserPortal/Posts/Posts.test.tsx(5 hunks)src/screens/UserPortal/Posts/Posts.tsx(8 hunks)src/utils/interfaces.ts(4 hunks)
💤 Files with no reviewable changes (1)
- .env.example
🧰 Additional context used
🪛 GitHub Check: CodeQL
src/components/OrgPostCard/OrgPostCard.tsx
[warning] 557-557: DOM text reinterpreted as HTML
DOM text is reinterpreted as HTML without escaping meta-characters.
[warning] 585-585: DOM text reinterpreted as HTML
DOM text is reinterpreted as HTML without escaping meta-characters.
src/components/UserPortal/PostCard/PostCard.tsx
[warning] 606-606: DOM text reinterpreted as HTML
DOM text is reinterpreted as HTML without escaping meta-characters.
[warning] 613-613: DOM text reinterpreted as HTML
DOM text is reinterpreted as HTML without escaping meta-characters.
src/components/UserPortal/StartPostModal/StartPostModal.tsx
[warning] 170-170: DOM text reinterpreted as HTML
DOM text is reinterpreted as HTML without escaping meta-characters.
🪛 Biome
src/components/UserPortal/PostCard/PostCard.test.tsx
[error] 666-666: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 902-902: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/components/UserPortal/StartPostModal/StartPostModal.test.tsx
[error] 222-225: Disallow duplicate setup and teardown hooks.
Disallow beforeEach duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
🔇 Additional comments (29)
src/components/UserPortal/CommentCard/CommentCard.module.css (1)
17-22: LGTM! Clean and well-structured CSS for creator images.
The new CSS rule appropriately handles avatar styling with proper sizing and aspect ratio maintenance.
src/components/UserPortal/PromotedPost/PromotedPost.tsx (1)
28-28: LGTM: Test ID addition
The addition of the data-testid attribute is a good improvement for testing capabilities.
src/screens/UserPortal/Posts/Posts.module.css (1)
130-130: Verify the migration of removed styles.
The change from 25px to 2rem for border-radius is a good improvement for responsive design. However, several core styles (background, border, color) were removed from .startPostBtn.
Let's verify where these styles are maintained now:
✅ Verification successful
Let me gather more information about the styling changes by searching for related files and components.
Let me try one more search to find the Posts component and its styling:
Based on the gathered information, I can now generate the final response:
The removed styles are handled by Bootstrap's Button component
The .startPostBtn class now only needs the border-radius property because:
- The button is rendered using Bootstrap's
Buttoncomponent (<Button variant="light" className={styles.startPostBtn}.../>) - Core styles are provided by Bootstrap's
btnandbtn-lightclasses - Additional styling is applied through Bootstrap utility classes:
w-100,text-start,text-muted,py-3,d-flex,align-items-center,gap-2, andborder
The change from 25px to 2rem for border-radius is appropriate as it maintains consistency with the responsive design approach used throughout the component.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential style definitions for startPostBtn
rg -l "startPostBtn" --type css
rg -l "start-post-btn" --type css
# Search for potential component styling
ast-grep --pattern 'styled.$_`
$$$
border-radius: $_;
$$$
`'
Length of output: 180
Script:
#!/bin/bash
# Search for StartPostBtn component
rg -l "StartPostBtn" --type-add 'jsx:*.{jsx,tsx}' --type jsx
# Look for styled-components usage in Posts directory
fd . src/screens/UserPortal/Posts -e tsx -e jsx --exec rg -l "styled\."
# Check for common style imports in Posts.tsx/jsx
fd Posts.tsx Posts.jsx -x rg "import.*style" {}
# Look for any global styles or theme files
fd "theme|style|design" -e ts -e js -e css
Length of output: 360
Script:
#!/bin/bash
# Find and examine Posts component file
fd Posts -e tsx -e jsx -x cat {}
# Look for any MUI or other UI library imports that might handle styling
fd Posts -e tsx -e jsx -x rg "import.*@mui|import.*@material" {}
# Check for any style-related imports in Posts directory
fd . src/screens/UserPortal/Posts -e tsx -e jsx -x rg "import.*css|import.*style" {}
Length of output: 22895
src/components/UserPortal/PostCard/PostCard.module.css (2)
22-24: LGTM! Improved creator section styling
The changes to creator section styling improve consistency by:
- Maintaining vertical alignment of elements
- Using margin instead of padding for spacing
- Adding proper circular avatar styling
Also applies to: 35-39
190-196: LGTM! Well-structured media container
The new media container provides proper centering and overflow handling, following good CSS practices.
src/components/UserPortal/CommentCard/CommentCard.tsx (1)
20-20: LGTM! Well-structured interface change.
The addition of the optional image property to the creator object is clean and maintains backward compatibility.
public/locales/zh/translation.json (2)
1048-1052: Verify date format placeholder usage.
The translation for "postedOn" uses a placeholder {{date}} which is good practice for dynamic content. This ensures proper date formatting based on locale settings.
688-690: 🛠️ Refactor suggestion
Review translations for consistency across sections.
The error messages in the "orgPost" section have been translated appropriately, but they appear to be duplicated in other sections with slightly different translations. Consider using consistent translations for the same error messages across all sections.
Standardize these translations across all sections:
"orgPost": {
- "noTitleAndDescription": "无法创建内容为空的帖子。",
- "postCreationFailed": "创建帖子失败。"
+ "noTitleAndDescription": "标题和描述不能为空。",
+ "postCreationFailed": "创建帖子失败。"
}This matches the translation used in the "orgPostCard" section (line 731) for better consistency.
Likely invalid or redundant comment.
public/locales/hi/translation.json (1)
1048-1052: LGTM! New translations for post card actions are clear and meaningful.
The added translations for post date formatting, update/deletion status, media selection, and comments are well-written and grammatically correct in Hindi.
public/locales/fr/translation.json (3)
688-690: LGTM! Error message translations are clear and consistent.
The added translations for post creation error messages are well-structured and maintain consistency with the application's error handling patterns.
730-733: LGTM! Post-related message translations are accurate.
The translations for API unavailability, post creation errors, and UI elements are properly formatted and maintain consistency with the rest of the application.
1070-1072: LGTM! Post creation message translations are consistent.
The translations for post creation success/failure messages maintain consistency with similar messages throughout the application.
public/locales/sp/translation.json (3)
689-691: LGTM! Clear and consistent error message translations.
The Spanish translations for navigation and error messages in the orgPost section are well-structured and meaningful.
1049-1053: LGTM! Well-formatted post-related message translations.
The translations for post-related messages are clear and properly use the date placeholder {{date}}. The error and status messages are meaningful in Spanish.
1071-1073: LGTM! Consistent error message translations.
The translations for post creation error messages maintain consistency with similar messages in other sections of the application.
src/components/UserPortal/StartPostModal/StartPostModal.tsx (1)
170-170: Ensure safe rendering of user-generated media
Ensure that rendering user-uploaded media does not introduce security vulnerabilities. Verify that previewUrl is safely handled and that user-supplied files cannot lead to XSS attacks.
While URL.createObjectURL is generally safe, it's important to validate the media files. Consider implementing client-side validation for file types and sizes.
🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 170-170: DOM text reinterpreted as HTML
DOM text is reinterpreted as HTML without escaping meta-characters.
src/screens/UserPortal/Posts/Posts.tsx (2)
367-375: Ensure Unique Keys in List Rendering
While rendering adContent, ensure that each child in a list has a unique key prop. Verify that post._id is unique across all advertisements.
If post._id is not guaranteed to be unique, consider using a combination of indexes or another unique identifier.
395-405: Ensure Proper Key Usage in Posts Mapping
When mapping over posts, ensure that each div has a unique key prop to prevent rendering issues.
Confirm that node._id is unique for each post.
-<div data-testid="post-card" key={node._id}>
+<div data-testid="post-card" key={`post-${node._id}`}>Optionally, prefix keys to reduce the risk of key collisions.
src/screens/OrganizationDashboard/OrganizationDashboardMocks.ts (6)
160-161: Confirm handling of null file values in posts
In the second post, the file field is set to null. Ensure that the application correctly handles posts without attached files and that setting file to null is acceptable according to the schema. This prevents potential runtime errors when accessing properties of a null file object.
Check that the GraphQL schema allows file to be null and that UI components gracefully handle posts without files.
167-167: Ensure the image field in creator aligns with the schema
As with the previous creator object, verify that including the image field is consistent with the application's User type definition.
180-181: Validate pagination cursors for accuracy
The startCursor and endCursor values have been updated. Confirm that these cursors accurately represent the first and last items in the edges array. This ensures that pagination functions correctly during testing.
Ensure that startCursor matches the cursor of the first post and endCursor matches that of the last post.
185-185: Confirm totalCount reflects the actual number of posts
The totalCount is set to 2, which should correspond to the number of posts provided in the edges array. Verifying this ensures consistency and correctness in pagination and total count displays.
116-116: Ensure the image field in creator aligns with the schema
The creator object now includes an image field. Verify that the User type in the application schema includes this image property. This ensures the frontend can display user images without encountering undefined fields.
Run the following script to check if the User type includes an image field:
#!/bin/bash
# Description: Verify if the `image` field exists in the `User` type.
# Test: Search for the `image` field in `User` type. Expect: Field exists.
ast-grep --pattern $'type User {
$$$
image: $_
$$$
}' --lang graphql schema.graphql95-110: Verify the file object structure matches the application's schema
The file object added to the post mock data includes fields like _id, fileName, mimeType, size, hash, uri, metadata, visibility, and status. Please ensure that the structure and field names align with the File type defined in the application's GraphQL schema. Consistency is crucial for accurate testing and to prevent potential mismatches during integration.
Run the following script to confirm that the File type in the schema matches the mock data structure:
src/screens/OrgPost/OrgPost.test.tsx (3)
1047-1051: Validate file type and size during file upload tests
The tests simulate uploading files but do not include validations for file type and size, which are important for file upload features. Consider adding assertions to check how the component handles invalid file types or oversized files.
Also applies to: 1125-1129, 1173-1177
591-806: 🛠️ Refactor suggestion
Ensure test cases cover error handling scenarios
While the test cases cover successful operations, they don't include scenarios where the fetch request fails or returns an error. Consider adding tests to verify how the component handles network errors or unsuccessful responses.
Would you like assistance in creating additional test cases for error handling?
54-59: Ensure correct mocking of useLocalstorage
The mock for utils/useLocalstorage returns an object with a getItem method but does not mock other potential methods like setItem. Ensure that all necessary methods are mocked to prevent unexpected errors during testing.
Run the following script to check for usage of setItem:
✅ Verification successful
Based on the comprehensive review of the codebase, I can now generate the final response:
The mock implementation of useLocalstorage is sufficient for this test file
The current mock implementation of useLocalstorage that returns only getItem is appropriate for this test file because:
- Looking at
OrgPost.tsx, the component only usesgetItemfromuseLocalstorageto retrieve the access token for post creation. - The component doesn't use
setItemor any other methods from the hook. - The test scenarios in
OrgPost.test.tsxfocus on post creation, searching, sorting, and pagination functionality, none of which require storage modification.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if `setItem` from `useLocalstorage` is used in the codebase.
# Test: Search for occurrences of `setItem` in combination with `useLocalstorage`.
rg -A 2 "useLocalstorage.*\.setItem"
Length of output: 38
Script:
#!/bin/bash
# Let's first check the implementation of useLocalstorage to understand its interface
rg -l "useLocalstorage"
# Then examine the actual implementation and usages
ast-grep --pattern 'export $_ function useLocalstorage() { $$$ }'
# Also search for any direct usage of setItem
rg "setItem"
# And check the test file content to understand the context better
cat src/screens/OrgPost/OrgPost.test.tsx
Length of output: 66288
Script:
#!/bin/bash
# Let's check the implementation of useLocalstorage to understand its interface
cat src/utils/useLocalstorage.ts
# And check if there are any usages of setItem in OrgPost.tsx
cat src/screens/OrgPost/OrgPost.tsx
Length of output: 22067
src/components/OrgPostCard/OrgPostCard.test.tsx (2)
117-120: Good practice: Clear mocks before each test
The addition of beforeEach to clear all mocks ensures that each test runs in isolation and prevents side effects from previous tests.
945-975: Ensure form validation handles empty inputs correctly
When testing form submission with empty spaces, verify that the form validation prevents submission and provides user feedback when fields contain only whitespace.
If the form allows submission with empty fields, consider updating the validation logic to trim inputs and check for empty strings.
| @@ -2,7 +2,7 @@ | |||
| . "$(dirname -- "$0")/_/husky.sh" | |||
|
|
|||
| # npm run format:fix | |||
| # npm run lint:fix | |||
| npm run lint:fix | |||
There was a problem hiding this comment.
💡 Codebase verification
Remove redundant lint:fix command from pre-commit hook
The lint:fix command is redundant as lint-staged already includes ESLint fixing for TypeScript files:
lint:fixruns:eslint --fix "**/*.{ts,tsx}"lint-stagedruns:eslint --fixon**/*.{ts, tsx, json}
The lint-staged configuration is more comprehensive as it:
- Handles the same file types as
lint:fix - Additionally includes JSON files
- Also runs Prettier formatting
- Only processes staged files instead of all files
🔗 Analysis chain
Consider potential redundancy with lint-staged.
The addition of npm run lint:fix before lint-staged might be redundant as lint-staged typically handles linting of staged files. Consider verifying if both commands are necessary or if they serve different purposes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the configuration of lint-staged and lint:fix to understand their scope
# Expected: Different configurations or purposes for each command
echo "Checking package.json for lint:fix and lint-staged configurations:"
cat package.json | jq '.scripts["lint:fix"], ."lint-staged"'
Length of output: 350
| .preview img { | ||
| width: 400px; | ||
| height: auto; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve image preview responsiveness.
The fixed width of 400px might cause overflow on smaller screens. Consider using max-width and relative units for better responsiveness.
.preview img {
- width: 400px;
+ width: 100%;
+ max-width: 400px;
height: auto;
}📝 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.
| .preview img { | |
| width: 400px; | |
| height: auto; | |
| } | |
| .preview img { | |
| width: 100%; | |
| max-width: 400px; | |
| height: auto; | |
| } |
| .preview video { | ||
| width: 400px; | ||
| height: auto; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance video preview styling and functionality.
- Apply the same responsive improvements as images
- Consider adding styles for video controls and play overlay
.preview video {
- width: 400px;
+ width: 100%;
+ max-width: 400px;
height: auto;
+ background-color: #000;
+ object-fit: contain;
}
+
+.preview video::-webkit-media-controls {
+ background-color: rgba(0, 0, 0, 0.5);
+}📝 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.
| .preview video { | |
| width: 400px; | |
| height: auto; | |
| .preview video { | |
| width: 100%; | |
| max-width: 400px; | |
| height: auto; | |
| background-color: #000; | |
| object-fit: contain; | |
| } | |
| .preview video::-webkit-media-controls { | |
| background-color: rgba(0, 0, 0, 0.5); | |
| } |
| .editPostImage, | ||
| .editPostVideo { | ||
| width: 100%; | ||
| object-fit: cover; | ||
| border-radius: 0; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add aspect ratio for consistent edit mode display
The edit mode styles should maintain the same aspect ratio as the display mode for consistency.
Apply this change:
.editPostImage,
.editPostVideo {
width: 100%;
+ aspect-ratio: 1.5;
object-fit: cover;
border-radius: 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.
| .editPostImage, | |
| .editPostVideo { | |
| width: 100%; | |
| object-fit: cover; | |
| border-radius: 0; | |
| .editPostImage, | |
| .editPostVideo { | |
| width: 100%; | |
| aspect-ratio: 1.5; | |
| object-fit: cover; | |
| border-radius: 0; |
| <div className={`${styles.creator} d-flex align-items-center gap-2`}> | ||
| {props?.creator?.image ? ( | ||
| <img src={props?.creator?.image} alt="creator image" /> | ||
| ) : ( | ||
| <AccountCircleIcon className="my-2" /> | ||
| )} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve image handling and accessibility.
While the implementation works, there are several areas for improvement:
- Remove redundant optional chaining since
creatoris required per interface - Add better error handling for images
- Enhance accessibility
Consider this improved implementation:
<div className={`${styles.creator} d-flex align-items-center gap-2`}>
- {props?.creator?.image ? (
- <img src={props?.creator?.image} alt="creator image" />
+ {props.creator.image ? (
+ <img
+ src={props.creator.image}
+ alt={`${creatorName}'s profile picture`}
+ onError={(e) => {
+ e.currentTarget.onerror = null;
+ e.currentTarget.src = ''; // Clear src
+ setImageError(true);
+ }}
+ />
) : (
- <AccountCircleIcon className="my-2" />
+ <AccountCircleIcon
+ className="my-2"
+ aria-label={`${creatorName}'s default profile icon`}
+ />
)}You'll need to add state for handling image errors:
const [imageError, setImageError] = React.useState(false);
src/screens/OrgPost/OrgPost.test.tsx
Outdated
| fireEvent.click(submitButton); | ||
|
|
There was a problem hiding this comment.
Wrap asynchronous operations with await act
When simulating user events that trigger asynchronous operations, it's important to wrap them with await act(async () => { ... }) to ensure all effects are processed.
Apply this diff to ensure proper handling of asynchronous events:
- fireEvent.click(submitButton);
+ await act(async () => {
+ fireEvent.click(submitButton);
+ });Also applies to: 1101-1103, 1150-1151
| // Mock getElementById for image input | ||
| const mockImageInput = document.createElement('input'); | ||
| mockImageInput.setAttribute('id', 'postImageUrl'); | ||
| mockImageInput.value = 'test.jpg'; | ||
| document.getElementById = jest.fn((id) => | ||
| id === 'postImageUrl' ? mockImageInput : null, | ||
| ); |
There was a problem hiding this comment.
Avoid overriding 'document.getElementById' globally
Overriding document.getElementById globally with jest.fn() can affect other tests and lead to unpredictable results. Use jest.spyOn to mock document.getElementById temporarily within the test scope.
Apply this diff to use jest.spyOn:
- document.getElementById = jest.fn((id) =>
- id === 'postImageUrl' ? mockImageInput : null,
- );
+ const getElementByIdSpy = jest.spyOn(document, 'getElementById').mockImplementation((id) =>
+ id === 'postImageUrl' ? mockImageInput : null,
+ );
+ // Cleanup after test
+ getElementByIdSpy.mockRestore();Also applies to: 841-844
| }); | ||
| }); |
There was a problem hiding this comment.
Test case contains no assertions
The test 'opens edit modal with video post and verifies state' does not contain any assertions after setting up the component and interactions. Ensure that the test includes assertions to verify the expected state changes or behaviors.
| jest.mock('utils/errorHandler', () => ({ | ||
| errorHandler: jest.fn(), | ||
| })); | ||
|
|
There was a problem hiding this comment.
Duplicate mocks of 'react-toastify' and 'errorHandler'
The jest.mock('react-toastify', ...) and jest.mock('utils/errorHandler', ...) are declared multiple times in the file. Having multiple mocks for the same module can lead to unexpected behavior. Consider consolidating each mock into a single declaration.
Apply this diff to remove the duplicate mocks:
- jest.mock('utils/errorHandler', () => ({
- errorHandler: jest.fn(),
- }));
- jest.mock('react-toastify', () => ({
- toast: {
- success: jest.fn(),
- warn: jest.fn(),
- error: jest.fn(),
- },
- }));📝 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.
| jest.mock('utils/errorHandler', () => ({ | |
| errorHandler: jest.fn(), | |
| })); |
| const clearButton = screen.getByTestId('closeimage'); | ||
| fireEvent.click(clearButton, { preventDefault: jest.fn() }); | ||
|
|
||
| // Verify preventDefault was called | ||
| expect(clearButton.onclick).toBeDefined(); | ||
| }); |
There was a problem hiding this comment.
Incorrect usage of 'preventDefault' in 'fireEvent.click'
The fireEvent.click method does not accept a preventDefault option. Instead, create a mock event object and verify that preventDefault is called within the event handler.
Apply this diff to mock the event properly:
- fireEvent.click(clearButton, { preventDefault: jest.fn() });
+ const mockEvent = { preventDefault: jest.fn() };
+ fireEvent.click(clearButton, mockEvent);
+ expect(mockEvent.preventDefault).toHaveBeenCalled();📝 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.
| const clearButton = screen.getByTestId('closeimage'); | |
| fireEvent.click(clearButton, { preventDefault: jest.fn() }); | |
| // Verify preventDefault was called | |
| expect(clearButton.onclick).toBeDefined(); | |
| }); | |
| const clearButton = screen.getByTestId('closeimage'); | |
| const mockEvent = { preventDefault: jest.fn() }; | |
| fireEvent.click(clearButton, mockEvent); | |
| expect(mockEvent.preventDefault).toHaveBeenCalled(); | |
| // Verify preventDefault was called | |
| expect(clearButton.onclick).toBeDefined(); | |
| }); |
| { | ||
| method: 'POST', | ||
| body: expect.any(FormData), | ||
| headers: { Authorization: 'Bearer dummy-token' }, |
Check failure
Code scanning / CodeQL
Hard-coded credentials Critical
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Outside diff range and nitpick comments (37)
src/components/TableLoader/TableLoader.test.tsx (1)
73-77: Consider enhancing error test specificity.The error test could be more specific by asserting the expected error message. This would ensure that the correct error is being thrown for the right reason.
Consider updating the test like this:
expect(() => { render( <BrowserRouter> <TableLoader {...props} /> </BrowserRouter>, - }).toThrow(); + }).toThrow('Either noOfCols or headerTitles must be provided');src/components/AddOn/core/AddOnRegister/AddOnRegister.test.tsx (1)
Line range hint
1-240: Consider improving test structure and reliability.A few suggestions to enhance the test suite:
- Standardize the provider setup across tests (currently mixing
MockedProviderandApolloProvider).- Replace hardcoded wait times (e.g.,
wait(3000)) with proper async utilities likewaitForto make tests more reliable.- Consider extracting mock data and setup into a separate test utilities file for better maintainability.
Example refactor for the wait time:
- await wait(3000); // Waiting for 3 seconds to reload the page + await waitFor( + () => { + expect(mockNavigate).toHaveBeenCalledWith(0); + }, + { timeout: 3000 } + );src/components/UserPortal/StartPostModal/StartPostModal.tsx (2)
13-13: Remove unused import.The
errorimport from 'console' appears to be unused in the code.-import { error } from 'console';
197-204: Enhance user feedback during post creation.Consider these UI improvements:
- Use translation for "Add Media" button text
- Add loading state during post creation
<Button variant="primary" onClick={handlePost} disabled={!postformState.postinfo.trim()} data-testid="createPostBtn" + className={isLoading ? 'loading' : ''} > - Post + {isLoading ? t('posting') : t('post')} </Button>Additional changes needed:
- Add
isLoadingstate- Set loading state during post creation
- Add translation keys for "posting" and "post"
src/components/OrgSettings/AgendaItemCategories/OrganizationAgendaCategory.test.tsx (2)
Line range hint
193-246: Uncomment and fix the error test case.The commented-out error test is valuable for ensuring proper error handling. However, it needs fixing as the assertion
expect(toast.error).toBeCalledWith()is incomplete - it's not verifying the error message.Consider uncommenting and updating the test with proper error message assertion:
- // await waitFor(() => { - // expect(toast.error).toBeCalledWith(); - // }); + await waitFor(() => { + expect(toast.error).toHaveBeenCalledWith( + expect.stringContaining('error') // Replace with actual error message from translations + ); + });
Line range hint
1-246: Add test coverage for file upload scenarios.Given that this PR introduces file upload features, consider adding test cases for:
- File upload validation
- File type restrictions
- Upload success/failure scenarios
- File deletion
Would you like me to help generate test cases for these scenarios?
src/components/UserPortal/Register/Register.test.tsx (2)
263-265: LGTM! Clear success case assertion with next steps.The success message effectively communicates the registration outcome and next steps to the user.
Consider adding a test case to verify the registration state/UI updates while waiting for admin approval.
Line range hint
1-265: Consider enhancing test coverage for edge cases.While the current test suite is solid, consider adding tests for:
- Network error handling
- Loading states during registration
- Specific validation patterns (email format, password requirements)
- Registration state management
This would provide more comprehensive coverage for error handling and user experience edge cases.
src/components/UserPortal/OrganizationCard/OrganizationCard.test.tsx (2)
Line range hint
27-121: Update mock data to support file upload operations.The current mock structure needs to be extended to support the new file upload features. Consider adding:
- Mocks for file upload mutations/queries
- Test cases for file validation errors
- Sample responses with file metadata
Here's a suggested addition to your mocks:
const FILE_UPLOAD_MOCKS = [ { request: { query: UPLOAD_ORGANIZATION_MEDIA, variables: { organizationId: '1', file: null, // Apollo will inject the actual file }, }, result: { data: { uploadOrganizationMedia: { _id: '1', fileName: 'test.jpg', fileType: 'image/jpeg', fileSize: 1024, url: 'https://example.com/test.jpg' } } } }, // Add error case mock { request: { query: UPLOAD_ORGANIZATION_MEDIA, variables: { organizationId: '1', file: null, }, }, error: new Error('File upload failed') } ];
Line range hint
123-130: Enhance async operation handling.While the current wait function works, consider using more reliable async utilities:
- Replace fixed timeouts with
waitFor- Add explicit assertions in wait conditions
- Use
findBy*queries where appropriateHere's a suggested improvement:
// Replace the wait function with more specific utilities import { waitFor } from '@testing-library/react'; // Instead of: await wait(); expect(element).toBeInTheDocument(); // Use: await waitFor(() => { expect(element).toBeInTheDocument(); }); // Or use findBy queries: const element = await screen.findByTestId('joinBtn');src/components/UserPortal/StartPostModal/StartPostModal.test.tsx (1)
14-22: Consider enhancing mock type safety and reusability.The toast mock could benefit from type safety, and common mocks could be extracted for reuse.
Consider applying these improvements:
// Create a new file: src/test/mocks/toast.mock.ts +import { toast as realToast } from 'react-toastify'; + +type ToastType = typeof realToast; + +export const mockToast: jest.Mocked<ToastType> = { + error: jest.fn(), + info: jest.fn(), + success: jest.fn(), + dismiss: jest.fn(), + // Add other methods as needed +} as jest.Mocked<ToastType>; + +jest.mock('react-toastify', () => ({ + toast: mockToast +}));src/components/AgendaItems/AgendaItemsContainer.test.tsx (1)
Line range hint
472-472: Implement missing test cases for drag and drop functionality.The TODO comment indicates that test coverage for drag and drop functionality (lines 172-202) is missing. This is important to ensure the reliability of this key user interaction.
Would you like me to help generate test cases for the drag and drop functionality? I can provide examples using
react-beautiful-dndtesting patterns.src/screens/UserPortal/Events/Events.test.tsx (1)
Line range hint
405-408: Remove or restore commented-out test code.These commented lines appear to be part of the calendar view test. Either restore the test functionality or remove the comments to maintain code cleanliness.
- // await wait(); - - // userEvent.click(screen.getByTestId('modeChangeBtn')); - // userEvent.click(screen.getByTestId('modeBtn1'));src/components/RecurrenceOptions/RecurrenceOptions.test.tsx (1)
Line range hint
1-586: Consider refactoring test suite for better maintainability.While the test coverage is comprehensive, consider these improvements:
- Extract common setup code into a helper function to reduce duplication
- Create constants for configuration values (e.g.,
30000timeout)- Break down long test cases into smaller, focused scenarios using
describeblocksExample refactor for common setup:
const setupTest = async () => { const rendered = render( <MockedProvider addTypename={false} link={link}> <BrowserRouter> <Provider store={store}> <LocalizationProvider dateAdapter={AdapterDayjs}> <ThemeProvider theme={theme}> <I18nextProvider i18n={i18n}> <OrganizationEvents /> </I18nextProvider> </ThemeProvider> </LocalizationProvider> </Provider> </BrowserRouter> </MockedProvider> ); await wait(); await waitFor(() => { expect(screen.getByTestId('createEventModalBtn')).toBeInTheDocument(); }); return rendered; };src/components/Advertisements/core/AdvertisementRegister/AdvertisementRegister.test.tsx (1)
Line range hint
1-653: Consider refactoring test setup for better maintainability.The test file has good coverage but could benefit from some refactoring:
- Extract common setup code into beforeEach/beforeAll hooks
- Create a helper function for managing timers (useFakeTimers/useRealTimers)
- Consider using test.each for similar test cases with different inputs
This would reduce code duplication and make the tests more maintainable.
src/components/OrgPostCard/OrgPostCard.tsx (4)
151-157: Optimize useEffect dependenciesThe useEffect hook should include all external values in its dependency array to prevent stale closures.
useEffect(() => { setPostFormState({ posttitle: postTitle, postinfo: postInfo, mediaFile: null, pinned: pinned, }); setPreviewUrl(postPhoto || postVideo); - }, []); + }, [postTitle, postInfo, postPhoto, postVideo, pinned]);
236-236: Remove console.log statementDebug statements should not be present in production code.
- console.log(data);
237-239: Enhance error handlingThe error handling could be more specific by checking for different HTTP status codes.
- if (!response.ok) { - throw new Error(data?.error || t('postCreationFailed')); - } + if (response.status === 413) { + throw new Error(t('fileTooLarge')); + } else if (response.status === 415) { + throw new Error(t('unsupportedFileType')); + } else if (!response.ok) { + throw new Error(data?.error || t('postCreationFailed')); + }
Line range hint
282-367: Simplify media preview renderingThe nested conditional rendering can be simplified for better maintainability.
- {postVideo && ( - // video card - )} - {postPhoto ? ( - // photo card - ) : !postVideo ? ( - // default card - ) : ( - '' - )} + {(() => { + if (postVideo) return renderVideoCard(); + if (postPhoto) return renderPhotoCard(); + return renderDefaultCard(); + })()}Consider extracting the card rendering logic into separate components for better maintainability.
src/components/RecurrenceOptions/CustomRecurrence.test.tsx (1)
Line range hint
1-724: Consider improving test maintainability.While the tests are comprehensive, there are a few opportunities for improvement:
- Extract common setup code into beforeEach blocks
- Create test helper functions for repetitive actions like setting up the event form
- Consider using a custom render function that includes all providers
Example implementation:
// test-utils.tsx const AllTheProviders = ({ children }) => { return ( <MockedProvider addTypename={false} link={link}> <BrowserRouter> <Provider store={store}> <LocalizationProvider dateAdapter={AdapterDayjs}> <ThemeProvider theme={theme}> <I18nextProvider i18n={i18nForTest}> {children} </I18nextProvider> </ThemeProvider> </LocalizationProvider> </Provider> </BrowserRouter> </MockedProvider> ); }; const customRender = (ui: React.ReactElement) => { return render(ui, { wrapper: AllTheProviders }); }; // Helper function const fillEventForm = async (formData) => { userEvent.type(screen.getByPlaceholderText(/Enter Title/i), formData.title); userEvent.type( screen.getByPlaceholderText(/Enter Description/i), formData.description ); // ... other form fields };This would simplify your tests to:
describe('Testing recurring events with custom recurrence patterns', () => { beforeEach(async () => { customRender(<OrganizationEvents />); await wait(); }); test('Creating a daily recurring event', async () => { await waitFor(() => { expect(screen.getByTestId('createEventModalBtn')).toBeInTheDocument(); }); userEvent.click(screen.getByTestId('createEventModalBtn')); await fillEventForm(formData); // ... rest of the test }); });src/components/UserPortal/PostCard/PostCard.tsx (2)
94-99: Consider using a discriminated union type for media stateInstead of managing separate state variables for media type and file, consider using a discriminated union type for better type safety and state management.
-const [editedMedia, setEditedMedia] = useState<File | null>(null); -const [editedMediaType, setEditedMediaType] = useState<'image' | 'video' | null>(null); +type MediaState = + | { type: null; file: null } + | { type: 'image' | 'video'; file: File }; +const [mediaState, setMediaState] = useState<MediaState>({ type: null, file: null });
653-653: Add loading state to edit buttonThe edit button should reflect the loading state to prevent multiple submissions.
<Button size="sm" variant="success" className="px-4" data-testid="editPostBtn" + disabled={isEditing} onClick={handleEditPost} > - {t('editPost')} + {isEditing ? t('saving') : t('editPost')} </Button>src/screens/UserPortal/Settings/Settings.tsx (6)
Line range hint
31-31: Rename component to follow React naming conventionReact components should use PascalCase naming convention. Rename
settingstoSettings.-export default function settings(): JSX.Element { +export default function Settings(): JSX.Element {
Line range hint
39-47: Optimize resize handler with debounceThe resize handler could trigger frequent re-renders. Consider debouncing the handler for better performance.
+import { debounce } from 'lodash'; + const handleResize = (): void => { if (window.innerWidth <= 820) { setHideDrawer(!hideDrawer); } }; + +const debouncedResize = debounce(handleResize, 250); useEffect(() => { handleResize(); - window.addEventListener('resize', handleResize); + window.addEventListener('resize', debouncedResize); return () => { - window.removeEventListener('resize', handleResize); + window.removeEventListener('resize', debouncedResize); + debouncedResize.cancel(); }; }, []);
Line range hint
290-307: Add file validation for image uploadsThe image upload handler should validate file size and type before processing. Also, consider adding loading state during image conversion.
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) + if (file.size > 5 * 1024 * 1024) { + toast.error(t('fileSizeError')); + return; + } + + // Validate file type + if (!file.type.startsWith('image/')) { + toast.error(t('fileTypeError')); + return; + } + + setUserDetails(prev => ({ ...prev, isLoading: true })); const image = await convertToBase64(file); - setUserDetails({ ...userDetails, image }); + setUserDetails(prev => ({ ...prev, image, isLoading: false })); } }}
Line range hint
108-134: Add loading state for user details updateThe update handler should show loading state during the API call to prevent multiple submissions and improve user experience.
+const [isUpdating, setIsUpdating] = useState(false); const handleUpdateUserDetails = async (): Promise<void> => { try { + setIsUpdating(true); let updatedUserDetails = { ...userDetails }; if (updatedUserDetails.image === originalImageState.current) { updatedUserDetails = { ...updatedUserDetails, image: '' }; } const { data } = await updateUserDetails({ variables: updatedUserDetails, }); if (data) { toast.success(tCommon('updatedSuccessfully', { item: 'Profile' }) as string); setTimeout(() => { window.location.reload(); }, 500); const userFullName = `${userDetails.firstName} ${userDetails.lastName}`; setItem('name', userFullName); } } catch (error: unknown) { errorHandler(t, error); } finally { + setIsUpdating(false); } };
Line range hint
246-257: Add phone number validation and formattingThe phone number input should include proper validation and formatting to ensure correct data entry.
+import { parsePhoneNumberFromString, AsYouType } from 'libphonenumber-js'; +const formatPhoneNumber = (value: string, country: string): string => { + const formatter = new AsYouType(country); + return formatter.input(value); +}; +const validatePhoneNumber = (value: string, country: string): boolean => { + const phoneNumber = parsePhoneNumberFromString(value, country); + return phoneNumber ? phoneNumber.isValid() : false; +}; <Form.Control type="tel" id="phoneNo" placeholder="1234567890" value={userDetails.phoneNumber} - onChange={(e) => handleFieldChange('phoneNumber', e.target.value)} + onChange={(e) => { + const formatted = formatPhoneNumber(e.target.value, userDetails.country); + handleFieldChange('phoneNumber', formatted); + }} + isInvalid={userDetails.phoneNumber && !validatePhoneNumber(userDetails.phoneNumber, userDetails.country)} className={`${styles.cardControl}`} data-testid="inputPhoneNumber" /> +{userDetails.phoneNumber && !validatePhoneNumber(userDetails.phoneNumber, userDetails.country) && ( + <Form.Control.Feedback type="invalid"> + {t('invalidPhoneNumber')} + </Form.Control.Feedback> +)}
Line range hint
466-478: Add form validation before submissionAdd form validation to ensure required fields are filled and valid before allowing submission.
+const validateForm = (): boolean => { + const required = ['firstName', 'lastName', 'email']; + const hasEmptyRequired = required.some(field => !userDetails[field]); + const hasInvalidPhone = userDetails.phoneNumber && + !validatePhoneNumber(userDetails.phoneNumber, userDetails.country); + return !hasEmptyRequired && !hasInvalidPhone; +}; <Button onClick={handleUpdateUserDetails} + disabled={!validateForm() || isUpdating} data-testid="updateUserBtn" className={`${styles.cardButton}`} > - {tCommon('saveChanges')} + {isUpdating ? tCommon('saving') : tCommon('saveChanges')} </Button>src/components/UserPortal/PostCard/PostCard.test.tsx (3)
370-370: Use a test constant instead of hard-coded tokenReplace the hard-coded Bearer token with a constant defined in a test utilities file. This improves maintainability and follows security best practices.
- headers: { Authorization: 'Bearer dummy-token' }, + headers: { Authorization: `Bearer ${TEST_AUTH_TOKEN}` },🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 370-370: Hard-coded credentials
The hard-coded value "Bearer dummy-token" is used as authorization header.
912-912: Fix typo in error messageThere's a typo in the error message text.
- 'Please select and image or video file.', + 'Please select an image or video file.',
688-690: Refactor duplicate cleanup codeThe URL.createObjectURL cleanup code is duplicated. Consider extracting it into a helper function and using optional chaining as suggested by the linter.
+ const cleanupObjectURL = () => { + global.URL?.createObjectURL = jest.fn(); + }; - if (global.URL && global.URL.createObjectURL) { - global.URL.createObjectURL = jest.fn(); - } + cleanupObjectURL();Also applies to: 937-939
🧰 Tools
🪛 Biome
[error] 688-688: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/screens/OrgPost/OrgPost.test.tsx (2)
17-29: Consider using a more type-safe mock implementation for react-i18next.The current mock implementation could be improved by adding proper TypeScript types and handling edge cases.
jest.mock('react-i18next', () => ({ useTranslation: () => ({ - t: (key: string) => key, + t: (key: string, options?: Record<string, unknown>) => { + if (typeof key !== 'string') return ''; + return options ? `${key} ${JSON.stringify(options)}` : key; + }, i18n: { - changeLanguage: () => new Promise(() => {}), + changeLanguage: () => Promise.resolve(), }, }), initReactI18next: { type: '3rdParty', init: () => {}, }, I18nextProvider: ({ children }: { children: React.ReactNode }) => children, }));
66-119: Consider extracting mock data to a separate file.The mock data is well-structured but could be moved to a separate test utilities file to improve maintainability and reusability across different test files.
Consider creating a new file
src/screens/OrgPost/__mocks__/mockData.tsand moving the mock data there:// src/screens/OrgPost/__mocks__/mockData.ts export const mockPostsData = { organizations: [ { posts: { edges: [ { node: { _id: '1', title: 'Test Post 1', // ... rest of the mock data }, cursor: 'cursor1', }, ], pageInfo: { // ... pageInfo mock data }, totalCount: 1, }, }, ], };src/components/OrgPostCard/OrgPostCard.test.tsx (1)
573-612: Good test coverage for pin toggle edge cases.The test properly verifies behavior when no data is returned from the mutation. Consider adding a test for network failures as well.
Add a test case for network failure:
test('should handle network error when toggling pin', async () => { const networkErrorMock = { request: { query: TOGGLE_PINNED_POST, variables: { id: '12' }, }, error: new Error('Network error'), }; const errorLink = new StaticMockLink([networkErrorMock], true); render( <MockedProvider link={errorLink} addTypename={false}> <I18nextProvider i18n={i18nForTest}> <OrgPostCard {...mockProps} /> </I18nextProvider> </MockedProvider>, ); userEvent.click(screen.getByTestId('cardStructure')); userEvent.click(screen.getByTestId('moreiconbtn')); userEvent.click(screen.getByTestId('pinpostBtn')); await waitFor(() => { expect(errorHandler).toHaveBeenCalledWith( expect.anything(), new Error('Network error'), ); }); });src/components/UsersTableItem/UserTableItem.test.tsx (1)
506-507: Consider consolidating navigation assertions.The code tests both
window.location.replaceandmockNavgatePushfor the same navigation action. While thorough, this might be redundant as they serve the same purpose. Consider choosing one navigation method based on your application's primary navigation strategy.- expect(window.location.replace).toHaveBeenCalledWith('/orgdash/abc'); - expect(mockNavgatePush).toHaveBeenCalledWith('/orgdash/abc'); + expect(mockNavgatePush).toHaveBeenCalledWith('/orgdash/abc');src/screens/UserPortal/Chat/Chat.test.tsx (2)
Line range hint
1548-1587: Enhance test coverage for direct chat creation.The current test only verifies UI element presence. Consider adding the following test scenarios:
- Successful chat creation with selected user
- Validation of required fields
- Error handling scenarios
- Verification of API calls
Example additions:
test('create new direct chat', async () => { // ... existing setup ... // Test user selection const userSelect = await screen.findByTestId('userSelect'); fireEvent.change(userSelect, { target: { value: 'Test User' } }); expect(screen.getByText('Test User')).toBeInTheDocument(); // Test successful submission fireEvent.click(submitBtn); await waitFor(() => { expect(screen.getByText('Chat created successfully')).toBeInTheDocument(); }); // Test validation fireEvent.click(submitBtn); await waitFor(() => { expect(screen.getByText('Please select a user')).toBeInTheDocument(); }); });
Line range hint
1588-1628: Improve group chat creation test coverage.The test only checks basic UI rendering. For comprehensive coverage, add tests for:
- Group name validation
- Member selection (minimum/maximum members)
- Successful group creation
- Error scenarios
Example improvements:
test('create new group chat', async () => { // ... existing setup ... // Test group name validation const groupNameInput = await screen.findByTestId('groupName'); fireEvent.change(groupNameInput, { target: { value: '' } }); fireEvent.click(submitBtn); expect(screen.getByText('Group name is required')).toBeInTheDocument(); // Test member selection const memberSelect = await screen.findByTestId('memberSelect'); fireEvent.change(memberSelect, { target: { value: ['user1', 'user2'] } }); expect(screen.getAllByTestId('selected-member')).toHaveLength(2); // Test successful submission fireEvent.change(groupNameInput, { target: { value: 'Test Group' } }); fireEvent.click(submitBtn); await waitFor(() => { expect(screen.getByText('Group created successfully')).toBeInTheDocument(); }); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (25)
src/GraphQl/Queries/fundQueries.ts(0 hunks)src/components/AddOn/core/AddOnRegister/AddOnRegister.test.tsx(1 hunks)src/components/Advertisements/core/AdvertisementRegister/AdvertisementRegister.test.tsx(4 hunks)src/components/AgendaCategory/AgendaCategoryContainer.test.tsx(2 hunks)src/components/AgendaItems/AgendaItemsContainer.test.tsx(1 hunks)src/components/EventListCard/EventListCard.test.tsx(8 hunks)src/components/OrgPostCard/OrgPostCard.test.tsx(15 hunks)src/components/OrgPostCard/OrgPostCard.tsx(17 hunks)src/components/OrgSettings/AgendaItemCategories/OrganizationAgendaCategory.test.tsx(1 hunks)src/components/RecurrenceOptions/CustomRecurrence.test.tsx(2 hunks)src/components/RecurrenceOptions/RecurrenceOptions.test.tsx(2 hunks)src/components/TableLoader/TableLoader.test.tsx(1 hunks)src/components/UserPortal/OrganizationCard/OrganizationCard.test.tsx(1 hunks)src/components/UserPortal/PostCard/PostCard.test.tsx(24 hunks)src/components/UserPortal/PostCard/PostCard.tsx(13 hunks)src/components/UserPortal/Register/Register.test.tsx(7 hunks)src/components/UserPortal/StartPostModal/StartPostModal.test.tsx(1 hunks)src/components/UserPortal/StartPostModal/StartPostModal.tsx(3 hunks)src/components/UsersTableItem/UserTableItem.test.tsx(3 hunks)src/screens/OrgPost/OrgPost.test.tsx(2 hunks)src/screens/OrganizationDashboard/OrganizationDashboard.tsx(0 hunks)src/screens/OrganizationEvents/OrganizationEvents.test.tsx(3 hunks)src/screens/UserPortal/Chat/Chat.test.tsx(2 hunks)src/screens/UserPortal/Events/Events.test.tsx(2 hunks)src/screens/UserPortal/Settings/Settings.tsx(1 hunks)
💤 Files with no reviewable changes (2)
- src/GraphQl/Queries/fundQueries.ts
- src/screens/OrganizationDashboard/OrganizationDashboard.tsx
🧰 Additional context used
🪛 GitHub Check: CodeQL
src/components/OrgPostCard/OrgPostCard.tsx
[warning] 556-556: DOM text reinterpreted as HTML
DOM text is reinterpreted as HTML without escaping meta-characters.
[warning] 584-584: DOM text reinterpreted as HTML
DOM text is reinterpreted as HTML without escaping meta-characters.
src/components/UserPortal/PostCard/PostCard.test.tsx
[failure] 370-370: Hard-coded credentials
The hard-coded value "Bearer dummy-token" is used as authorization header.
src/components/UserPortal/PostCard/PostCard.tsx
[warning] 617-617: DOM text reinterpreted as HTML
DOM text is reinterpreted as HTML without escaping meta-characters.
[warning] 624-624: DOM text reinterpreted as HTML
DOM text is reinterpreted as HTML without escaping meta-characters.
src/components/UserPortal/StartPostModal/StartPostModal.tsx
[warning] 169-169: DOM text reinterpreted as HTML
DOM text is reinterpreted as HTML without escaping meta-characters.
🪛 Biome
src/components/UserPortal/PostCard/PostCard.test.tsx
[error] 688-688: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 937-937: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/components/UserPortal/StartPostModal/StartPostModal.test.tsx
[error] 232-235: Disallow duplicate setup and teardown hooks.
Disallow beforeEach duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
🔇 Additional comments (39)
src/components/TableLoader/TableLoader.test.tsx (1)
76-76: LGTM! Good simplification of the error assertion.
The change from .toThrowError() to .toThrow() is a good improvement as it's more concise while maintaining the same functionality.
src/components/AddOn/core/AddOnRegister/AddOnRegister.test.tsx (2)
158-158: LGTM! Good update to Jest assertion method.
The change from toBeCalledWith to toHaveBeenCalledWith aligns with Jest best practices and improves test readability.
158-158: 🛠️ Refactor suggestion
Consider adding test coverage for file upload functionality.
Given that this PR introduces file upload features for user and admin portals (as per PR objectives), consider adding test cases to verify:
- File upload validation
- Successful file upload scenarios
- Error handling for failed uploads
- File size and type restrictions
Let's check if there are any file upload related tests in the codebase:
src/components/UserPortal/StartPostModal/StartPostModal.tsx (2)
32-40: LGTM! Well-structured state management.
The state management is well-organized with proper TypeScript types and clean initialization.
181-181: Previous review comment about useRef is still valid.
The current implementation using document.getElementById should be replaced with useRef as suggested in the previous review.
src/components/OrgSettings/AgendaItemCategories/OrganizationAgendaCategory.test.tsx (1)
188-190: LGTM! Good improvement in test assertion.
The change to toHaveBeenCalledWith follows Jest's preferred syntax and improves readability.
src/components/UserPortal/Register/Register.test.tsx (3)
107-107: LGTM! Updated to modern Jest assertion syntax.
The change from toBeCalledWith to toHaveBeenCalledWith follows Jest's recommended assertion syntax.
127-127: LGTM! Consistent validation error assertions.
The toast error assertions have been uniformly updated to use modern Jest syntax while maintaining consistent error messaging across all validation scenarios.
Also applies to: 148-148, 172-172, 198-198
226-228: LGTM! Clear password mismatch error assertion.
The multi-line assertion format improves readability while maintaining consistent modern Jest syntax.
src/components/UserPortal/OrganizationCard/OrganizationCard.test.tsx (2)
2-2: LGTM! Clean import consolidation.
The consolidation of testing library imports improves code organization while maintaining all necessary functionality.
Line range hint 1-359: Add test coverage for new file upload features.
Given that this PR introduces file upload capabilities, consider adding test cases for:
- Rendering organization cards with uploaded media
- Handling different media types (images, videos)
- Media upload error scenarios
- Media preview functionality
This will ensure the new features are properly tested and maintain reliability.
Let's verify the component's media-related props and functionality:
Would you like me to help draft the additional test cases?
src/components/UserPortal/StartPostModal/StartPostModal.test.tsx (1)
79-305: LGTM! Test suite structure is well-organized.
The test suite has good coverage of the main functionality with well-structured test cases. The component is properly unmounted after each test by the testing library.
🧰 Tools
🪛 Biome
[error] 232-235: Disallow duplicate setup and teardown hooks.
Disallow beforeEach duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
src/components/AgendaCategory/AgendaCategoryContainer.test.tsx (1)
272-274: LGTM! Improved test assertion readability.
The changes to use toHaveBeenCalledWith instead of toBeCalledWith improve readability while maintaining the same functionality. This update is consistent with similar changes across other test files in the codebase.
Also applies to: 367-369
src/components/AgendaItems/AgendaItemsContainer.test.tsx (1)
374-376: LGTM! Improved assertion syntax.
The update to use toHaveBeenCalledWith instead of toBeCalledWith follows Jest's best practices for mock function assertions.
src/screens/OrganizationEvents/OrganizationEvents.test.tsx (3)
285-285: LGTM! Assertion method updated to follow Jest's best practices.
The change from toBeCalledWith to toHaveBeenCalledWith aligns with Jest's recommended assertion methods for spies and mocks.
455-455: LGTM! Success message assertion is consistent.
The assertion for successful event creation maintains consistency with the updated Jest assertion method.
Line range hint 1-463: Consider adding test coverage for file upload scenarios.
Given that this PR introduces file upload capabilities for posts, consider adding test cases to verify:
- File upload during event creation
- Validation of file types and sizes
- Error handling for failed uploads
Let's check if there are any file upload-related tests in the codebase:
#!/bin/bash
# Description: Search for existing file upload test coverage
# Expected: Find test files that cover file upload functionality
# Look for file upload related test files
rg -l "upload.*test|test.*upload" --type typescript --type tsx
# Search for file upload related test cases
ast-grep --pattern 'test($_, $_) {
$$$
upload
$$$
}'src/screens/UserPortal/Events/Events.test.tsx (3)
339-341: LGTM! Good update to modern Jest practices.
The change from toBeCalledWith to toHaveBeenCalledWith aligns with current Jest best practices and provides better error messages.
382-384: LGTM! Maintains consistency in assertion methods.
The update to toHaveBeenCalledWith maintains consistency with modern Jest practices across test cases.
Line range hint 1-577: Consider adding tests for file upload functionality.
Given that this PR introduces file upload capabilities for user and admin feeds (as mentioned in the PR objectives), consider adding test cases to verify:
- File upload functionality
- File type validation
- Upload size limits
- Error handling for failed uploads
Let me check if there are any file upload related tests in other files:
Would you like me to help create test cases for the file upload functionality?
src/components/RecurrenceOptions/RecurrenceOptions.test.tsx (2)
456-456: LGTM! Standardized assertion method.
The change from toBeCalledWith to toHaveBeenCalledWith aligns with Jest's preferred assertion style. Both methods are functionally equivalent.
578-578: LGTM! Consistent assertion style.
Similar to the previous change, this standardizes the assertion method to toHaveBeenCalledWith.
src/components/Advertisements/core/AdvertisementRegister/AdvertisementRegister.test.tsx (3)
252-254: LGTM! Improved test assertion.
The update to use toHaveBeenCalledWith follows Jest's best practices for mock function assertions.
468-470: LGTM! Clear error handling for date validation.
The error message is descriptive and the test properly verifies the validation behavior.
595-597: LGTM! Consistent error handling in edit mode.
The error handling maintains consistency with the create mode and properly validates date ranges.
src/components/OrgPostCard/OrgPostCard.tsx (1)
26-26: LGTM: Good addition of refetch prop
The addition of the refetch prop enhances the component's reusability by allowing parent components to refresh their data after post updates.
src/components/RecurrenceOptions/CustomRecurrence.test.tsx (1)
584-584: LGTM! Good update to use the preferred Jest assertion method.
The change from toBeCalledWith to toHaveBeenCalledWith follows Jest's best practices. While both methods work, toHaveBeenCalledWith is the canonical name and provides better readability.
Also applies to: 712-712
src/components/UserPortal/PostCard/PostCard.tsx (4)
24-26: LGTM: New imports for media handling icons
The added imports align well with the new media handling functionality.
48-48: LGTM: Interface update for creator image
The addition of the image property to the creator interface is consistent with the new media handling capabilities.
571-602: LGTM: Well-structured media management UI
The media management UI is well-organized with clear buttons for changing and removing media. The conditional rendering based on media type is implemented correctly.
613-644:
Address potential XSS vulnerabilities in media preview
The media preview URLs should be sanitized to prevent XSS attacks. Consider implementing URL validation or using a sanitization library.
Apply this fix to validate URLs:
+const isValidUrl = (url: string): boolean => {
+ try {
+ new URL(url);
+ return true;
+ } catch {
+ return false;
+ }
+};
+const sanitizeUrl = (url: string): string => {
+ if (!isValidUrl(url)) {
+ console.error('Invalid URL detected');
+ return '';
+ }
+ return url;
+};
-src={mediaPreviewUrl}
+src={sanitizeUrl(mediaPreviewUrl)}src/components/EventListCard/EventListCard.test.tsx (1)
365-365: LGTM! Consistent update of Jest assertions.
The changes correctly update the assertion method from toBeCalledWith to toHaveBeenCalledWith, which is the preferred method name in Jest. The test suite demonstrates excellent practices with comprehensive coverage of component functionality.
Also applies to: 418-418, 462-462, 699-699, 765-765, 826-826, 866-866, 924-924
src/components/UserPortal/PostCard/PostCard.test.tsx (1)
597-940: LGTM! Comprehensive test coverage
The new test cases thoroughly cover media handling functionality, including file type validation, API responses, and error scenarios. The tests are well-structured and follow testing best practices.
🧰 Tools
🪛 Biome
[error] 688-688: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 937-937: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/screens/OrgPost/OrgPost.test.tsx (2)
565-575: Previous comment about duplicate renderWithProviders is still valid.
Also applies to: 426-436
589-597: LGTM: Basic component rendering test is well implemented.
The test case properly checks for loading state and essential UI elements.
src/components/OrgPostCard/OrgPostCard.test.tsx (1)
115-118: LGTM: Good practice to clear mocks before each test.
The addition of beforeEach to clear all mocks helps prevent test pollution and ensures each test starts with a clean slate.
src/components/UsersTableItem/UserTableItem.test.tsx (3)
502-502: LGTM: Updated toast assertion following Jest best practices.
The change from toBeCalledWith to toHaveBeenCalledWith improves test clarity and follows Jest's recommended assertion style.
696-696: LGTM: Consistent update to toast assertion.
The change maintains consistency with the updated Jest assertion style used throughout the file.
723-724: Same feedback applies: Consider consolidating navigation assertions.
This is the same pattern of redundant navigation testing as discussed earlier.
| <video controls data-testid="videoPreview"> | ||
| <source | ||
| src={previewUrl} | ||
| type={postformState.mediaFile.type} | ||
| /> | ||
| ({t('tag')}) | ||
| </video> |
There was a problem hiding this comment.
Fix potential XSS vulnerability in video source.
The video source URL should be properly sanitized before being rendered to prevent XSS attacks.
<video controls data-testid="videoPreview">
<source
- src={previewUrl}
+ src={previewUrl ? encodeURI(previewUrl) : ''}
type={postformState.mediaFile.type}
/>
({t('tag')})
</video>Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 169-169: DOM text reinterpreted as HTML
DOM text is reinterpreted as HTML without escaping meta-characters.
| const response = await fetch( | ||
| `${process.env.REACT_APP_TALAWA_REST_URL}/create-post`, | ||
| { | ||
| method: 'POST', | ||
| body: formData, | ||
| headers: { Authorization: `Bearer ${accessToken}` }, | ||
| }, | ||
| }); | ||
| /* istanbul ignore next */ | ||
| ); | ||
|
|
||
| const data = await response.json(); | ||
| if (!response.ok) { | ||
| throw new Error(data?.error || t('postCreationFailed')); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and add request timeout.
Consider these improvements for better error handling and user experience:
- Add request timeout
- Add file size validation
- Handle specific HTTP status codes
const response = await fetch(
`${process.env.REACT_APP_TALAWA_REST_URL}/create-post`,
{
method: 'POST',
body: formData,
headers: { Authorization: `Bearer ${accessToken}` },
+ signal: AbortSignal.timeout(30000), // 30 second timeout
},
);
+const MAX_FILE_SIZE = 5 * 1024 * 1024; // 5MB
+if (postformState.mediaFile && postformState.mediaFile.size > MAX_FILE_SIZE) {
+ throw new Error(t('fileTooBig'));
+}
const data = await response.json();
if (!response.ok) {
+ switch (response.status) {
+ case 413:
+ throw new Error(t('fileTooBig'));
+ case 415:
+ throw new Error(t('unsupportedFileType'));
+ default:
throw new Error(data?.error || t('postCreationFailed'));
+ }
}Committable suggestion skipped: line range outside the PR's diff.
| beforeEach(() => { | ||
| jest.clearAllMocks(); | ||
| // Reset fetch mock | ||
| (global.fetch as jest.Mock).mockReset(); | ||
| // Reset createObjectURL mock | ||
| mockCreateObjectURL.mockClear(); | ||
| // Reset revokeObjectURL mock | ||
| mockRevokeObjectURL.mockClear(); | ||
| }); |
There was a problem hiding this comment.
Merge duplicate beforeEach hooks.
There are two separate beforeEach hooks that should be consolidated.
Apply this change:
describe('StartPostModal Component', () => {
const mockClick = jest.fn();
beforeEach(() => {
jest.clearAllMocks();
// Reset fetch mock
(global.fetch as jest.Mock).mockReset();
// Reset createObjectURL mock
mockCreateObjectURL.mockClear();
// Reset revokeObjectURL mock
mockRevokeObjectURL.mockClear();
+ // Setup document.getElementById mock
+ document.getElementById = jest.fn().mockReturnValue({ click: mockClick });
});
- beforeEach(() => {
- jest.clearAllMocks();
- document.getElementById = jest.fn().mockReturnValue({ click: mockClick });
- });
// ... rest of the tests
});Also applies to: 232-235
| it('should correctly append file to FormData', async () => { | ||
| // Mock API response | ||
| (global.fetch as jest.Mock).mockResolvedValueOnce({ | ||
| ok: true, | ||
| json: async () => ({ success: true }), | ||
| }); | ||
|
|
||
| // Setup and render | ||
| renderStartPostModal(); | ||
|
|
||
| // Select file | ||
| const fileInput = screen.getByTestId('modalFileInput'); | ||
| const testFile = new File(['test'], 'test.png', { type: 'image/png' }); | ||
| await userEvent.upload(fileInput, testFile); | ||
|
|
||
| // Trigger post (which will create FormData and append file) | ||
| const postInput = screen.getByTestId('postInput'); | ||
| await userEvent.type(postInput, 'test'); | ||
| await userEvent.click(screen.getByTestId('createPostBtn')); | ||
|
|
||
| // Verify file was appended to FormData | ||
| await waitFor(() => { | ||
| const formData = (global.fetch as jest.Mock).mock.calls[0][1] | ||
| .body as FormData; | ||
| expect(formData.get('file')).toEqual(testFile); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add tests for file upload edge cases.
The current test suite lacks coverage for important file upload scenarios.
Add these test cases:
it('should handle file size limit exceeded', async () => {
renderStartPostModal();
const fileInput = screen.getByTestId('modalFileInput');
const largeFile = new File(['x'.repeat(5 * 1024 * 1024)], 'large.png', {
type: 'image/png'
});
await userEvent.upload(fileInput, largeFile);
expect(toast.error).toHaveBeenCalledWith(
expect.stringContaining('file size exceeds')
);
});
it('should handle invalid file type', async () => {
renderStartPostModal();
const fileInput = screen.getByTestId('modalFileInput');
const invalidFile = new File(['test'], 'test.txt', {
type: 'text/plain'
});
await userEvent.upload(fileInput, invalidFile);
expect(toast.error).toHaveBeenCalledWith(
expect.stringContaining('file type not supported')
);
});
it('should handle network error during upload', async () => {
(global.fetch as jest.Mock).mockRejectedValueOnce(
new Error('Network error')
);
renderStartPostModal();
const fileInput = screen.getByTestId('modalFileInput');
const file = new File(['test'], 'test.png', { type: 'image/png' });
await userEvent.upload(fileInput, file);
const postInput = screen.getByTestId('postInput');
await userEvent.type(postInput, 'test post');
await userEvent.click(screen.getByTestId('createPostBtn'));
await waitFor(() => {
expect(toast.error).toHaveBeenCalledWith(
expect.stringContaining('network error')
);
});
});| expect(toast.warning).toHaveBeenCalledWith('Title can not be blank!'); | ||
| expect(toast.warning).toHaveBeenCalledWith('Description can not be blank!'); | ||
| expect(toast.warning).toHaveBeenCalledWith('Location can not be blank!'); |
There was a problem hiding this comment.
💡 Codebase verification
Warning messages should be internationalized
The code uses hardcoded English strings for validation messages despite having i18n setup in place. The file already imports and uses useTranslation for other messages, but these validation messages are not using the translation system.
- In
src/screens/OrganizationEvents/OrganizationEvents.tsx, replace hardcoded strings with translation keys:toast.warning('Title can not be blank!'); toast.warning('Description can not be blank!'); toast.warning('Location can not be blank!');
🔗 Analysis chain
LGTM! Validation messages are properly tested.
The assertions correctly verify that appropriate warning messages are displayed for blank title, description, and location. The update to toHaveBeenCalledWith maintains consistency with Jest's best practices.
Let's verify that these validation messages match the translations:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the validation messages match the translation strings
# Expected: The messages should be defined in the translation files
rg -l "Title can not be blank|Description can not be blank|Location can not be blank" --type yaml
Length of output: 160
Script:
#!/bin/bash
# Let's search for these validation messages in the codebase to find their source
# and any translation files that might contain them
# Search for the validation messages in all files
rg -l "Title can not be blank|Description can not be blank|Location can not be blank"
# Also search for potential translation keys or constants
rg -l "TITLE_BLANK|DESCRIPTION_BLANK|LOCATION_BLANK"
# Look for translation files
fd -e json -e yaml locale
Length of output: 283
Script:
#!/bin/bash
# Let's examine the actual implementation in OrganizationEvents.tsx to see how these messages are used
rg -A 5 -B 5 "Title can not be blank|Description can not be blank|Location can not be blank" src/screens/OrganizationEvents/OrganizationEvents.tsx
# Also check if there's any i18n/translation setup or constants in the file
rg "import.*i18n|useTranslation|messages" src/screens/OrganizationEvents/OrganizationEvents.tsx
Length of output: 825
| fireEvent.click(submitButton); | ||
|
|
||
| // Verify fetch call does not include file in FormData | ||
| await waitFor(() => { | ||
| expect(global.fetch).toHaveBeenCalled(); | ||
| const fetchCall = (global.fetch as jest.Mock).mock.calls[0]; | ||
| const [, { body }] = fetchCall; | ||
|
|
||
| expect(body instanceof FormData).toBe(true); | ||
| expect(body.get('file')).toBeFalsy(); | ||
| }); | ||
| }); | ||
|
|
||
| it('handles file removal correctly', async () => { | ||
| renderComponent(); | ||
|
|
||
| // Open create post modal | ||
| await waitFor(() => { | ||
| const createButton = screen.getByTestId('createPostModalBtn'); | ||
| fireEvent.click(createButton); | ||
| }); | ||
|
|
||
| // Add file | ||
| const file = new File(['test file content'], 'test-image.png', { | ||
| type: 'image/png', | ||
| }); | ||
| const fileInput = screen.getByTestId('addMediaField'); | ||
|
|
||
| fireEvent.change(fileInput, { target: { files: [file] } }); | ||
|
|
||
| // Verify file preview is shown | ||
| await waitFor(() => { | ||
| expect(screen.getByTestId('mediaPreview')).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| // Remove file | ||
| const removeButton = screen.getByTestId('mediaCloseButton'); | ||
| fireEvent.click(removeButton); | ||
|
|
||
| // Verify preview is removed | ||
| expect(screen.queryByTestId('mediaPreview')).not.toBeInTheDocument(); | ||
|
|
||
| // Fill required fields and submit | ||
| const titleInput = screen.getByTestId('modalTitle'); | ||
| const infoInput = screen.getByTestId('modalinfo'); | ||
|
|
||
| fireEvent.change(titleInput, { target: { value: 'Test Title' } }); | ||
| fireEvent.change(infoInput, { target: { value: 'Test Content' } }); | ||
|
|
||
| const submitButton = screen.getByTestId('createPostBtn'); | ||
| fireEvent.click(submitButton); | ||
|
|
||
| // Verify fetch call does not include removed file | ||
| await waitFor(() => { | ||
| expect(global.fetch).toHaveBeenCalled(); | ||
| const fetchCall = (global.fetch as jest.Mock).mock.calls[0]; | ||
| const [, { body }] = fetchCall; | ||
|
|
||
| expect(body instanceof FormData).toBe(true); | ||
| expect(body.get('file')).toBeFalsy(); | ||
| }); | ||
| }); | ||
|
|
||
| it('handles different file types correctly', async () => { | ||
| renderComponent(); | ||
|
|
||
| // Open create post modal | ||
| await waitFor(() => { | ||
| const createButton = screen.getByTestId('createPostModalBtn'); | ||
| fireEvent.click(createButton); | ||
| }); | ||
|
|
||
| // Test image file | ||
| const imageFile = new File(['image content'], 'test-image.png', { | ||
| type: 'image/png', | ||
| }); | ||
| const fileInput = screen.getByTestId('addMediaField'); | ||
|
|
||
| fireEvent.change(fileInput, { target: { files: [imageFile] } }); | ||
|
|
||
| // Verify image preview | ||
| await waitFor(() => { | ||
| expect(screen.getByTestId('imagePreview')).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| // Remove image | ||
| const removeButton = screen.getByTestId('mediaCloseButton'); | ||
| fireEvent.click(removeButton); | ||
|
|
||
| // Test video file | ||
| const videoFile = new File(['video content'], 'test-video.mp4', { | ||
| type: 'video/mp4', | ||
| }); | ||
| fireEvent.change(fileInput, { target: { files: [videoFile] } }); | ||
|
|
||
| // Verify video preview | ||
| await waitFor(() => { | ||
| expect(screen.getByTestId('videoPreview')).toBeInTheDocument(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using test.each for file handling test cases.
The media file handling test cases have similar patterns and could be refactored using test.each to reduce code duplication and improve maintainability.
describe('Media File Handling in Post Creation', () => {
const testCases = [
{
name: 'appends file to FormData when media file is present',
file: new File(['test file content'], 'test-image.png', { type: 'image/png' }),
expectFile: true,
},
{
name: 'does not append file to FormData when no media file is present',
file: null,
expectFile: false,
},
];
test.each(testCases)('$name', async ({ file, expectFile }) => {
renderComponent();
// Open modal and fill form
await act(async () => {
fireEvent.click(screen.getByTestId('createPostModalBtn'));
});
// Add file if present
if (file) {
const fileInput = screen.getByTestId('addMediaField');
await act(async () => {
fireEvent.change(fileInput, { target: { files: [file] } });
});
}
// Submit and verify
await act(async () => {
fireEvent.click(screen.getByTestId('createPostBtn'));
});
await waitFor(() => {
const fetchCall = (global.fetch as jest.Mock).mock.calls[0];
const [, { body }] = fetchCall;
expect(body instanceof FormData).toBe(true);
expect(!!body.get('file')).toBe(expectFile);
});
});
});| // Submit form | ||
| const submitButton = screen.getByTestId('createPostBtn'); | ||
| fireEvent.click(submitButton); | ||
|
|
||
| // Verify fetch call includes file in FormData | ||
| await waitFor(() => { | ||
| expect(global.fetch).toHaveBeenCalled(); | ||
| const fetchCall = (global.fetch as jest.Mock).mock.calls[0]; | ||
| const [, { body }] = fetchCall; | ||
|
|
||
| expect(body instanceof FormData).toBe(true); | ||
| expect(body.get('file')).toBeTruthy(); | ||
| expect(body.get('file')).toEqual(file); | ||
| }); |
There was a problem hiding this comment.
Improve async operation handling in form submission test.
The form submission test should use await act() to properly handle async state updates.
- // Submit form
- const submitButton = screen.getByTestId('createPostBtn');
- fireEvent.click(submitButton);
+ // Submit form
+ await act(async () => {
+ const submitButton = screen.getByTestId('createPostBtn');
+ fireEvent.click(submitButton);
+ });📝 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.
| // Submit form | |
| const submitButton = screen.getByTestId('createPostBtn'); | |
| fireEvent.click(submitButton); | |
| // Verify fetch call includes file in FormData | |
| await waitFor(() => { | |
| expect(global.fetch).toHaveBeenCalled(); | |
| const fetchCall = (global.fetch as jest.Mock).mock.calls[0]; | |
| const [, { body }] = fetchCall; | |
| expect(body instanceof FormData).toBe(true); | |
| expect(body.get('file')).toBeTruthy(); | |
| expect(body.get('file')).toEqual(file); | |
| }); | |
| // Submit form | |
| await act(async () => { | |
| const submitButton = screen.getByTestId('createPostBtn'); | |
| fireEvent.click(submitButton); | |
| }); | |
| // Verify fetch call includes file in FormData | |
| await waitFor(() => { | |
| expect(global.fetch).toHaveBeenCalled(); | |
| const fetchCall = (global.fetch as jest.Mock).mock.calls[0]; | |
| const [, { body }] = fetchCall; | |
| expect(body instanceof FormData).toBe(true); | |
| expect(body.get('file')).toBeTruthy(); | |
| expect(body.get('file')).toEqual(file); | |
| }); |
| test('opens edit modal with video post and verifies state', async () => { | ||
| setItem('id', '123'); | ||
|
|
||
| const videoProps = { | ||
| ...props, | ||
| postPhoto: null, | ||
| postVideo: 'test-video.mp4', | ||
| pinned: true, | ||
| }; | ||
|
|
||
| render( | ||
| <MockedProvider addTypename={false} link={link}> | ||
| <I18nextProvider i18n={i18nForTest}> | ||
| <OrgPostCard {...videoProps} /> | ||
| </I18nextProvider> | ||
| </MockedProvider>, | ||
| ); | ||
|
|
||
| await wait(); | ||
|
|
||
| // Open the edit modal | ||
| userEvent.click(screen.getByTestId('cardVid')); | ||
| userEvent.click(screen.getByTestId('moreiconbtn')); | ||
| userEvent.click(screen.getByTestId('editPostModalBtn')); | ||
|
|
||
| // Verify the modal is open | ||
| expect(screen.getByTestId('modalOrganizationHeader')).toBeInTheDocument(); | ||
|
|
||
| // Verify that the preview URL is set to the video URL | ||
| const videoPreview = screen.getByTestId('videoPreview'); | ||
| const source = videoPreview.querySelector('source'); | ||
| expect(source).toHaveAttribute('src', 'test-video.mp4'); | ||
|
|
||
| // Verify form state is initialized correctly | ||
| expect(screen.getByTestId('updateTitle')).toHaveValue(videoProps.postTitle); | ||
| expect(screen.getByTestId('updateText')).toHaveValue(videoProps.postInfo); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add assertions to verify component state.
The test "opens edit modal with video post and verifies state" lacks assertions for modal state and video preview visibility.
Add these assertions:
// Add after line 1267
expect(screen.getByTestId('editPostModal')).toBeInTheDocument();
expect(videoPreview).toBeVisible();
expect(screen.getByTestId('closePreview')).toBeInTheDocument();| test('prevents default event behavior', () => { | ||
| render( | ||
| <MockedProvider> | ||
| <I18nextProvider i18n={i18nForTest}> | ||
| <OrgPostCard {...mockProps} /> | ||
| </I18nextProvider> | ||
| </MockedProvider>, | ||
| ); | ||
|
|
||
| // Open edit modal to access the clear button | ||
| fireEvent.click(screen.getByAltText('image')); | ||
| fireEvent.click(screen.getByTestId('moreiconbtn')); | ||
| fireEvent.click(screen.getByTestId('editPostModalBtn')); | ||
|
|
||
| // Get the clear button and simulate click with mock event | ||
| const clearButton = screen.getByTestId('closeimage'); | ||
| fireEvent.click(clearButton, { preventDefault: jest.fn() }); | ||
|
|
||
| // Verify preventDefault was called | ||
| expect(clearButton.onclick).toBeDefined(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix event mocking in preventDefault test.
The current implementation incorrectly mocks preventDefault. The event object should be created and passed properly.
Apply this fix:
test('prevents default event behavior', () => {
render(
<MockedProvider>
<I18nextProvider i18n={i18nForTest}>
<OrgPostCard {...mockProps} />
</I18nextProvider>
</MockedProvider>
);
// Open edit modal
fireEvent.click(screen.getByAltText('image'));
fireEvent.click(screen.getByTestId('moreiconbtn'));
fireEvent.click(screen.getByTestId('editPostModalBtn'));
const clearButton = screen.getByTestId('closeimage');
const mockEvent = { preventDefault: jest.fn() };
fireEvent.click(clearButton, mockEvent);
expect(mockEvent.preventDefault).toHaveBeenCalled();
});| test('clears image input and resets related state', () => { | ||
| // Mock getElementById for image input | ||
| const mockImageInput = document.createElement('input'); | ||
| mockImageInput.setAttribute('id', 'postImageUrl'); | ||
| mockImageInput.value = 'test.jpg'; | ||
| document.getElementById = jest.fn((id) => | ||
| id === 'postImageUrl' ? mockImageInput : null, | ||
| ); | ||
|
|
||
| render( | ||
| <MockedProvider> | ||
| <I18nextProvider i18n={i18nForTest}> | ||
| <OrgPostCard {...mockProps} /> | ||
| </I18nextProvider> | ||
| </MockedProvider>, | ||
| ); | ||
|
|
||
| // Open edit modal to access the clear button | ||
| fireEvent.click(screen.getByAltText('image')); | ||
| fireEvent.click(screen.getByTestId('moreiconbtn')); | ||
| fireEvent.click(screen.getByTestId('editPostModalBtn')); | ||
|
|
||
| // Trigger clear media input | ||
| const clearButton = screen.getByTestId('closeimage'); | ||
| fireEvent.click(clearButton); | ||
|
|
||
| // Check if image input value is cleared | ||
| expect(mockImageInput.value).toBe(''); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use jest.spyOn for document.getElementById mocks.
The current implementation directly overwrites document.getElementById which can affect other tests. Use jest.spyOn instead and ensure cleanup.
Apply this pattern to both image and video input tests:
test('clears image input and resets related state', () => {
const mockImageInput = document.createElement('input');
mockImageInput.setAttribute('id', 'postImageUrl');
mockImageInput.value = 'test.jpg';
const spy = jest.spyOn(document, 'getElementById').mockImplementation((id) =>
id === 'postImageUrl' ? mockImageInput : null
);
// ... rest of the test ...
spy.mockRestore(); // Clean up
});Also applies to: 827-862
palisadoes
added
the
ignore-sensitive-files-pr
|
Please fix the failing tests, the conflicting file and make code rabbit approve the PR |
|
This pull request did not get any activity in the past 10 days and will be closed in 180 days if no update occurs. Please verify it has no conflicts with the develop branch and rebase if needed. Mention it now if you need help or give permission to other people to finish your work. |
|
@chandel-aman Please fix the conflicting files and tests. You'll have to merge your changes into the new |
|
This pull request did not get any activity in the past 10 days and will be closed in 180 days if no update occurs. Please verify it has no conflicts with the develop branch and rebase if needed. Mention it now if you need help or give permission to other people to finish your work. |
|
I have to close the PR.
When reopening, please apply your changes to the |
What kind of change does this PR introduce?
Feature
Issue Number:
Fixes #2400
Did you add tests for your changes?
Yes
Summary
This PR brings about these changes:
Does this PR introduce a breaking change?
No
Other information
GSoC'24 project: Improved files uploads and security.
Have you read the contributing guide?
Yes
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
Tests
Style