feat: add CRUDModalTemplate foundation with tests #5763
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
No significant changes currently retry |
Our Pull Request Approval ProcessThis PR will be reviewed according to our: Your PR may be automatically closed if:
Thanks for contributing! |
Contributor
WalkthroughIntroduces a new CRUD modal template component with supporting types, styles, and test suite. Includes a React functional component rendering a modal with header, body, and footer sections; handling loading and error states; and supporting keyboard shortcuts. Defines base props interface and exports public APIs. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
src/shared-components/CRUDModalTemplate/CRUDModalTemplate.module.csssrc/shared-components/CRUDModalTemplate/CRUDModalTemplate.spec.tsxsrc/shared-components/CRUDModalTemplate/CRUDModalTemplate.tsxsrc/shared-components/CRUDModalTemplate/index.tssrc/shared-components/CRUDModalTemplate/types.ts
🧰 Additional context used
🧠 Learnings (22)
📓 Common learnings
Learnt from: palisadoes
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-12-18T02:57:44.185Z
Learning: Phase 1 issues created for foundations: #5083 (ProfileAvatarDisplay), #5084 (AdminPortal structure), #5085 (UserTableRow), #5086 (BaseModal), #5087 (EmptyState), #5088 (LoadingState).
Learnt from: Ansingh0305
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-01T16:34:10.347Z
Learning: In talawa-admin parallel test sharding (PR #4565), Bootstrap modals render via portals to document.body, causing screen.getByRole('dialog') to find dialogs from ALL concurrent test shards. Solution: use unique data-testid per modal instance and query within that scope using within() helper, or use findAllByTestId and take last element (less robust, timing-dependent).
Learnt from: palisadoes
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-12-20T23:32:16.510Z
Learning: In PalisadoesFoundation/talawa-admin bulk issue creation prompts, each issue must include two developer-enablement sections: (1) Example Starter Code (types.ts, component scaffold, spec scaffold, and before→after for rollout), and (2) Learning Resources with issue-specific tutorials (framework/library/a11y/testing). Titles must include the Phase, all issues assigned to palisadoes-bot, only the refactor label, and both upstream (Blocked by) and downstream (Blocks) links using actual GitHub issue numbers. Also apply a special duplicate check for LoadingStateWrapper vs #5088.
Learnt from: palisadoes
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-12-19T05:13:00.954Z
Learning: When creating ErrorBoundaryWrapper implementation plans for PalisadoesFoundation/talawa-admin, include: (1) Complete sample implementation with interface.ts, component, and tests, (2) Top 15-20 candidate files table with complexity, LOC impact, error patterns, priority, and feature area, (3) Two detailed before/after examples (simple and complex cases) showing code diffs and benefits, (4) Phased implementation plan with summary table showing files, PRs, hours, dependencies, and priority, (5) GitHub-ready issue templates with markdown format including: Issue Title, Problem Statement, Proposed Solution, Files to Modify (checkbox format), Implementation Approach (step-by-step), Dependencies (Blocked by/Blocks/Related with #numbers), Acceptance Criteria (checkbox format), Effort Estimate (hours breakdown), Testing Requirements (specific test cases), and Success Criteria, (6) Timeline with Gantt chart options (conservative vs aggressive), (7) Success metrics (before/after comparison). This ...
Learnt from: Chaitanya1672
Repo: PalisadoesFoundation/talawa-admin PR: 2049
File: src/screens/OrganizationActionItems/ActionItemUpdateModal.tsx:112-138
Timestamp: 2024-10-08T16:13:41.996Z
Learning: The `istanbul ignore next` comments in the `ActionItemUpdateModal.tsx` file were added as part of a commit that introduced tests for the `ActionItemUpdateModal` component. Removing these comments and writing tests to cover the previously ignored lines is recommended to ensure code integrity and improve test coverage.
Learnt from: rawadhossain
Repo: PalisadoesFoundation/talawa-admin PR: 4865
File: src/components/EventRegistrantsModal/Modal/EventRegistrantsModal.spec.tsx:261-280
Timestamp: 2025-11-27T11:40:30.747Z
Learning: In Talawa Admin tests (e.g., src/components/EventRegistrantsModal/Modal/EventRegistrantsModal.spec.tsx), react-bootstrap Modal header close buttons don't support custom test IDs, so using getAllByRole('button', { name: /close/i })[0] to select the modal's primary close action is acceptable and stable, as the Modal component consistently exposes "Close" as the accessible label.
Learnt from: Ansingh0305
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-12T00:28:53.713Z
Learning: In talawa-admin PR #4565 continuation (PR #4664 planned), Phase 2A mock isolation strategy uses global vi.clearAllMocks() in afterEach (vitest.setup.ts) plus refactoring module-level vi.fn() into per-test beforeEach blocks across 6 parallelizable issues (2A.0 foundation, 2A.1-2A.4 directory-based fixes, 2A.5 validation). This approach addresses mock call history accumulation enabling maxConcurrency: 2 and 2.3-2.5x speedup. vi.restoreAllMocks() alternative considered for spy restoration during validation phase. Phase 2A (mocks) correctly separated from Phase 2B (DOM/Apollo).
Learnt from: palisadoes
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-12-21T01:21:06.670Z
Learning: In PalisadoesFoundation/talawa-admin, when creating bulk reusable-component rollout work, the maintainer expects not only the foundation component issues but also one adoption issue per affected screen/file (auto-discovered), each assigned to palisadoes-bot, labeled only 'refactor', titled with the Phase, linked to actual upstream blockers (e.g., #5248, #5088, and the foundation issue), and including an "Implementation Plan" link to the initiating comment plus "Example Starter Code" and "Learning Resources".
Learnt from: palisadoes
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-12-24T17:07:11.187Z
Learning: PalisadoesFoundation/talawa-admin (#5426): For the shared-components migration plan and its sub-issues, do not include sample code in the plan or issue bodies; keep concise checklists that state “move files to target directory,” “update importing files,” and “update tests.” Phase 3 adoption must be batched in groups of ≤10 files (by portal), and all issue titles must use the “SharedComp - ” prefix.
Learnt from: palisadoes
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-12-19T00:27:59.172Z
Learning: In the PalisadoesFoundation/talawa-admin repo, when rolling out multi-PR component work (e.g., ProfileAvatarDisplay), the maintainer expects: (1) comprehensive plan to be reposted on the foundation issue (#5083), (2) all phase issues cross-linked to both #5083 and the coordinating epic (#5130), and (3) a tracking checklist table added to #5130. Use idempotent markers "AUTO-PLAN", "AUTO-LINK", and "AUTO-TRACKING" to avoid duplicate comments.
Learnt from: meetulr
Repo: PalisadoesFoundation/talawa-admin PR: 2387
File: src/screens/ManageTag/ManageTag.tsx:58-97
Timestamp: 2024-10-30T13:38:29.300Z
Learning: In the `ManageTag` component (`src/screens/ManageTag/ManageTag.tsx`), extracting modal state management to a custom hook may require additional state modifications for the `toggle` and `show` functions.
Learnt from: palisadoes
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-12-06T11:50:22.732Z
Learning: Talawa Admin reusable components: Each shared component must expose its props via src/types/<Component>/interface.ts (no inline interfaces) and include a short TSDoc header describing purpose and props.
Learnt from: palisadoes
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-12-06T11:50:22.732Z
Learning: Talawa Admin reusable components: Define each shared component’s props in src/types/<Component>/interface.ts (no inline interfaces) and add a short TSDoc header describing purpose and props.
Learnt from: palisadoes
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-12-06T11:50:22.732Z
Learning: Talawa Admin reusable components: Each shared component’s props must be defined in src/types/<Component>/interface.ts (strict typing; avoid inline prop types or any).
Learnt from: palisadoes
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-12-06T11:50:22.732Z
Learning: Talawa Admin reusable components: Define each shared component’s props in src/types/<Component>/interface.ts (avoid inline interfaces) and add a brief TSDoc header describing purpose and props.
📚 Learning: 2025-11-27T11:40:30.747Z
Learnt from: rawadhossain
Repo: PalisadoesFoundation/talawa-admin PR: 4865
File: src/components/EventRegistrantsModal/Modal/EventRegistrantsModal.spec.tsx:261-280
Timestamp: 2025-11-27T11:40:30.747Z
Learning: In Talawa Admin tests (e.g., src/components/EventRegistrantsModal/Modal/EventRegistrantsModal.spec.tsx), react-bootstrap Modal header close buttons don't support custom test IDs, so using getAllByRole('button', { name: /close/i })[0] to select the modal's primary close action is acceptable and stable, as the Modal component consistently exposes "Close" as the accessible label.
Applied to files:
src/shared-components/CRUDModalTemplate/CRUDModalTemplate.spec.tsxsrc/shared-components/CRUDModalTemplate/CRUDModalTemplate.tsxsrc/shared-components/CRUDModalTemplate/types.tssrc/shared-components/CRUDModalTemplate/CRUDModalTemplate.module.css
📚 Learning: 2024-10-08T16:13:41.996Z
Learnt from: Chaitanya1672
Repo: PalisadoesFoundation/talawa-admin PR: 2049
File: src/screens/OrganizationActionItems/ActionItemUpdateModal.tsx:112-138
Timestamp: 2024-10-08T16:13:41.996Z
Learning: The `istanbul ignore next` comments in the `ActionItemUpdateModal.tsx` file were added as part of a commit that introduced tests for the `ActionItemUpdateModal` component. Removing these comments and writing tests to cover the previously ignored lines is recommended to ensure code integrity and improve test coverage.
Applied to files:
src/shared-components/CRUDModalTemplate/CRUDModalTemplate.spec.tsx
📚 Learning: 2025-12-06T11:50:22.732Z
Learnt from: palisadoes
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-12-06T11:50:22.732Z
Learning: Talawa Admin reusable components: Each shared component must have a corresponding Component.spec.tsx test file.
Applied to files:
src/shared-components/CRUDModalTemplate/CRUDModalTemplate.spec.tsx
📚 Learning: 2025-12-18T09:49:48.620Z
Learnt from: AdityaRathi-10
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-12-18T09:49:48.620Z
Learning: In PalisadoesFoundation/talawa-admin PostCard.spec.tsx, custom vi.mock for useLocalstorage with shared module-level store object violated test isolation in Shard 3. The correct approach: remove custom mock, use real useLocalStorage() hook in beforeEach, change afterEach from vi.restoreAllMocks() to vi.clearAllMocks() + localStorage.clear(). The real hook uses window.localStorage which is mocked by Vitest test environment globally.
Applied to files:
src/shared-components/CRUDModalTemplate/CRUDModalTemplate.spec.tsx
📚 Learning: 2025-11-28T16:02:31.814Z
Learnt from: rawadhossain
Repo: PalisadoesFoundation/talawa-admin PR: 4882
File: src/screens/UserPortal/Volunteer/UpcomingEvents/RecurringEventVolunteerModal.spec.tsx:69-109
Timestamp: 2025-11-28T16:02:31.814Z
Learning: In Talawa Admin test files (e.g., src/screens/UserPortal/Volunteer/UpcomingEvents/RecurringEventVolunteerModal.spec.tsx), developers prefer keeping date/time mocks (like vi.spyOn(Date.prototype, 'toLocaleDateString')) inline within individual test cases rather than consolidating them in beforeEach blocks, to maintain clarity and keep mocks close to the tests that use them, even if it results in some duplication.
Applied to files:
src/shared-components/CRUDModalTemplate/CRUDModalTemplate.spec.tsx
📚 Learning: 2025-11-01T16:34:10.347Z
Learnt from: Ansingh0305
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-01T16:34:10.347Z
Learning: In talawa-admin parallel test sharding (PR #4565), Bootstrap modals render via portals to document.body, causing screen.getByRole('dialog') to find dialogs from ALL concurrent test shards. Solution: use unique data-testid per modal instance and query within that scope using within() helper, or use findAllByTestId and take last element (less robust, timing-dependent).
Applied to files:
src/shared-components/CRUDModalTemplate/CRUDModalTemplate.spec.tsx
📚 Learning: 2025-11-30T23:13:22.697Z
Learnt from: Ansingh0305
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-30T23:13:22.697Z
Learning: In talawa-admin PR #4908, increasing test concurrency from maxConcurrency: 1 to 6-12 with TOTAL_SHARDS: 12 exposed three critical latent bugs: (1) EventsAttendedByUser.spec.tsx used wrong GraphQL query mock (EVENT_DETAILS vs EVENT_DETAILS_BASIC with incorrect variable names), (2) User.mocks.ts missing search/reset refetch scenarios causing "No more mocked responses" errors, (3) EventCalendar.spec.tsx UTC timezone bug where new Date().toISOString() caused date calculation mismatches in non-UTC timezones. These bugs were masked at lower concurrency but surfaced consistently under parallel execution stress-testing. Fix involved aligning mocks with actual component queries and explicit timezone-aware date construction.
Applied to files:
src/shared-components/CRUDModalTemplate/CRUDModalTemplate.spec.tsx
📚 Learning: 2025-08-17T07:39:34.255Z
Learnt from: syedali237
Repo: PalisadoesFoundation/talawa-admin PR: 4077
File: package.json:189-213
Timestamp: 2025-08-17T07:39:34.255Z
Learning: The Talawa Admin codebase primarily uses .spec.tsx/.spec.ts naming convention for unit tests, with Cypress tests using .cy.ts pattern. However, there is at least one .test.tsx file in the codebase, so NYC exclude patterns should include both .spec and .test patterns.
Applied to files:
src/shared-components/CRUDModalTemplate/CRUDModalTemplate.spec.tsx
📚 Learning: 2025-11-11T11:51:09.236Z
Learnt from: Ansingh0305
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-11-11T11:51:09.236Z
Learning: In talawa-admin PR #4664 (continuation of PR #4565 sharding work), global mock cleanup was implemented via vi.clearAllMocks() in vitest.setup.ts afterEach, along with Apollo Client warning suppression (temporary, pending follow-up PR) and test suite refactoring to per-test StaticMockLink instances. This test isolation strategy addresses mock state leakage that forced maxConcurrency: 1 in PR #4565, enabling future parallel test execution improvements and 3-4x speedup potential.
Applied to files:
src/shared-components/CRUDModalTemplate/CRUDModalTemplate.spec.tsx
📚 Learning: 2025-10-22T22:22:27.696Z
Learnt from: Jashan32
Repo: PalisadoesFoundation/talawa-admin PR: 4524
File: src/screens/Requests/Requests.spec.tsx:0-0
Timestamp: 2025-10-22T22:22:27.696Z
Learning: In talawa-admin tests using react-infinite-scroll-component (e.g., src/screens/Requests/Requests.spec.tsx), jsdom scroll is flaky. Prefer a resilient approach: try querying the container via document.querySelector('[data-testid="infinite-scroll-component"]') and fall back to window scroll if missing. Strengthen assertions by checking for page-2 items (e.g., "User9 Test") to ensure fetchMore actually ran instead of relying only on row counts.
Applied to files:
src/shared-components/CRUDModalTemplate/CRUDModalTemplate.spec.tsx
📚 Learning: 2025-11-27T11:41:54.843Z
Learnt from: rawadhossain
Repo: PalisadoesFoundation/talawa-admin PR: 4865
File: src/components/EventRegistrantsModal/Modal/EventRegistrantsModal.spec.tsx:302-328
Timestamp: 2025-11-27T11:41:54.843Z
Learning: In Talawa Admin tests (e.g., src/components/EventRegistrantsModal/Modal/EventRegistrantsModal.spec.tsx), using queryByLabelText with an HTMLElement cast for MUI Autocomplete inputs is acceptable because the Autocomplete input can mount asynchronously and isn't always available for strict getBy* queries at initial render. This pattern is stable for MUI Autocomplete components.
Applied to files:
src/shared-components/CRUDModalTemplate/CRUDModalTemplate.spec.tsx
📚 Learning: 2025-01-26T12:32:45.867Z
Learnt from: IITI-tushar
Repo: PalisadoesFoundation/talawa-admin PR: 3400
File: src/screens/UserPortal/Organizations/Organizations.spec.tsx:19-19
Timestamp: 2025-01-26T12:32:45.867Z
Learning: In React test files, avoid using React hooks outside component scope (including in static objects like mock data). Instead, initialize hooks inside describe blocks or extract the needed functionality without using hooks.
Applied to files:
src/shared-components/CRUDModalTemplate/CRUDModalTemplate.spec.tsx
📚 Learning: 2025-12-21T08:59:37.942Z
Learnt from: rawadhossain
Repo: PalisadoesFoundation/talawa-admin PR: 5287
File: src/shared-components/Recurrence/CustomRecurrenceModal.spec.tsx:78-84
Timestamp: 2025-12-21T08:59:37.942Z
Learning: In talawa-admin test files, ensure each spec file uses an explicit vi.clearAllMocks() in an afterEach block to guarantee test isolation. This should be present even if a global cleanup exists in vitest.setup.ts, as the linter enforces per-file hygiene. Apply this guideline to all spec files (e.g., src/**/*.spec.tsx) to satisfy ESLint/Vitest requirements and prevent bleed-over between tests.
Applied to files:
src/shared-components/CRUDModalTemplate/CRUDModalTemplate.spec.tsx
📚 Learning: 2024-10-30T13:38:29.300Z
Learnt from: meetulr
Repo: PalisadoesFoundation/talawa-admin PR: 2387
File: src/screens/ManageTag/ManageTag.tsx:58-97
Timestamp: 2024-10-30T13:38:29.300Z
Learning: In the `ManageTag` component (`src/screens/ManageTag/ManageTag.tsx`), extracting modal state management to a custom hook may require additional state modifications for the `toggle` and `show` functions.
Applied to files:
src/shared-components/CRUDModalTemplate/CRUDModalTemplate.tsxsrc/shared-components/CRUDModalTemplate/types.ts
📚 Learning: 2025-12-18T02:57:44.185Z
Learnt from: palisadoes
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-12-18T02:57:44.185Z
Learning: Refactoring standard: Use components/AdminPortal/** for admin-only UI (with mirrored src/types/AdminPortal/**), components/UserPortal/** for user-only UI, and components/<Name>/ at root for shared UI; all props in src/types/<Component>/interface.ts with brief TSDoc.
Applied to files:
src/shared-components/CRUDModalTemplate/CRUDModalTemplate.tsxsrc/shared-components/CRUDModalTemplate/types.tssrc/shared-components/CRUDModalTemplate/index.ts
📚 Learning: 2025-12-06T11:50:22.732Z
Learnt from: palisadoes
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-12-06T11:50:22.732Z
Learning: Talawa Admin reusable components: Define each shared component’s props in src/types/<Component>/interface.ts (avoid inline interfaces) and add a brief TSDoc header describing purpose and props.
Applied to files:
src/shared-components/CRUDModalTemplate/types.ts
📚 Learning: 2025-12-06T11:50:22.732Z
Learnt from: palisadoes
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-12-06T11:50:22.732Z
Learning: Talawa Admin reusable components: Define each shared component’s props in src/types/<Component>/interface.ts (no inline interfaces) and add a short TSDoc header describing purpose and props.
Applied to files:
src/shared-components/CRUDModalTemplate/types.ts
📚 Learning: 2025-12-06T11:50:22.732Z
Learnt from: palisadoes
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-12-06T11:50:22.732Z
Learning: Talawa Admin reusable components: Each shared component must expose its props via src/types/<Component>/interface.ts (no inline interfaces) and include a short TSDoc header describing purpose and props.
Applied to files:
src/shared-components/CRUDModalTemplate/types.ts
📚 Learning: 2025-12-14T21:12:48.274Z
Learnt from: Jashan32
Repo: PalisadoesFoundation/talawa-admin PR: 5009
File: src/types/Post/interface.ts:106-123
Timestamp: 2025-12-14T21:12:48.274Z
Learning: In talawa-admin, post-related interfaces (including component props like ICreatePostModalProps, InterfacePinnedPostsLayoutProps, InterfacePinnedPostCardProps, and data interfaces like InterfacePost, InterfacePostEdge) are centralized in src/types/Post/interface.ts to facilitate reuse across multiple post directories and components, following a functionality-based organization pattern similar to Event and User types.
Applied to files:
src/shared-components/CRUDModalTemplate/types.ts
📚 Learning: 2025-12-06T11:50:22.732Z
Learnt from: palisadoes
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-12-06T11:50:22.732Z
Learning: Talawa Admin reusable components: Each shared component’s props must be defined in src/types/<Component>/interface.ts (avoid inline prop interfaces).
Applied to files:
src/shared-components/CRUDModalTemplate/types.ts
📚 Learning: 2025-12-06T11:50:22.732Z
Learnt from: palisadoes
Repo: PalisadoesFoundation/talawa-admin PR: 0
File: :0-0
Timestamp: 2025-12-06T11:50:22.732Z
Learning: Talawa Admin reusable components: Each shared component’s props must be defined in src/types/<Component>/interface.ts (strict typing; avoid inline prop types or any).
Applied to files:
src/shared-components/CRUDModalTemplate/types.ts
🧬 Code graph analysis (1)
src/shared-components/CRUDModalTemplate/CRUDModalTemplate.tsx (1)
src/shared-components/CRUDModalTemplate/types.ts (1)
CrudModalBaseProps(1-22)
🔇 Additional comments (4)
src/shared-components/CRUDModalTemplate/index.ts (1)
1-2: LGTM with note: update after types relocation.The barrel export pattern is correct. Note that line 2 will need updating once the types are moved to
src/types/CRUDModalTemplate/interface.tsas flagged in the types.ts review.src/shared-components/CRUDModalTemplate/CRUDModalTemplate.tsx (2)
36-43: Verify keyboard interaction compatibility.The Modal has
keyboardprop (line 42) which enables react-bootstrap's built-in Escape key handling foronHide. The custom Enter handler (lines 22-33) adds to this. Ensure these don't conflict and that the behavior matches PR objectives for "Esc to close, Enter to submit."Test both keyboard shortcuts together to ensure:
- Escape triggers
onClose(via react-bootstrap's keyboard prop)- Enter triggers
onPrimary(via custom handler) only when appropriate- No double-firing or event bubbling issues
48-52: Consider showing children alongside error messages.Currently, when an
erroris present, thechildrencontent is still displayed (line 51 checks!loading, not!loading && !error). This may be intentional for showing validation errors alongside form fields, but verify this matches the intended UX. If errors should hide the form content, add&& !errorto the condition.src/shared-components/CRUDModalTemplate/CRUDModalTemplate.module.css (1)
1-57: CSS module is unused; remove it.The component uses only react-bootstrap components (
Modal,Button,Spinner,Alert) with their built-in styling and never importsCRUDModalTemplate.module.css. Delete this file or refactor to use custom elements with these styles instead.⛔ Skipped due to learnings
Learnt from: rawadhossain Repo: PalisadoesFoundation/talawa-admin PR: 4865 File: src/components/EventRegistrantsModal/Modal/EventRegistrantsModal.spec.tsx:261-280 Timestamp: 2025-11-27T11:40:30.747Z Learning: In Talawa Admin tests (e.g., src/components/EventRegistrantsModal/Modal/EventRegistrantsModal.spec.tsx), react-bootstrap Modal header close buttons don't support custom test IDs, so using getAllByRole('button', { name: /close/i })[0] to select the modal's primary close action is acceptable and stable, as the Modal component consistently exposes "Close" as the accessible label.Learnt from: meetulr Repo: PalisadoesFoundation/talawa-admin PR: 2387 File: src/screens/ManageTag/ManageTag.tsx:58-97 Timestamp: 2024-10-30T13:38:29.300Z Learning: In the `ManageTag` component (`src/screens/ManageTag/ManageTag.tsx`), extracting modal state management to a custom hook may require additional state modifications for the `toggle` and `show` functions.Learnt from: IITI-tushar Repo: PalisadoesFoundation/talawa-admin PR: 3400 File: src/screens/UserPortal/Settings/Settings.tsx:10-10 Timestamp: 2025-01-26T12:28:42.904Z Learning: Global CSS files like `app.module.css` can be imported directly without using the `styles` import pattern, as they are meant to apply styles globally rather than as CSS modules.
Comment on lines
+7
to
+53
| describe('CRUDModalTemplate', () => { | ||
| it('renders title and content when open', () => { | ||
| render( | ||
| <CRUDModalTemplate | ||
| open | ||
| title="Create Item" | ||
| onClose={() => {}} | ||
| > | ||
| <div>Modal Content</div> | ||
| </CRUDModalTemplate> | ||
| ); | ||
|
|
||
| expect(screen.getByText(/create item/i)).toBeInTheDocument(); | ||
| expect(screen.getByText(/modal content/i)).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('calls onClose when cancel button is clicked', async () => { | ||
| const onClose = vi.fn(); | ||
|
|
||
| render( | ||
| <CRUDModalTemplate | ||
| open | ||
| title="Create Item" | ||
| onClose={onClose} | ||
| /> | ||
| ); | ||
|
|
||
| await userEvent.click(screen.getByText(/cancel/i)); | ||
| expect(onClose).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('calls onPrimary when Enter key is pressed', async () => { | ||
| const onPrimary = vi.fn(); | ||
|
|
||
| render( | ||
| <CRUDModalTemplate | ||
| open | ||
| title="Create Item" | ||
| onClose={() => {}} | ||
| onPrimary={onPrimary} | ||
| /> | ||
| ); | ||
|
|
||
| await userEvent.keyboard('{Enter}'); | ||
| expect(onPrimary).toHaveBeenCalled(); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
Add afterEach cleanup and expand test coverage.
Two issues:
-
Missing required cleanup: Per project standards, each spec file must include
afterEach(() => { vi.clearAllMocks(); });to ensure test isolation. -
Insufficient test coverage: The PR objectives specify >90% coverage and accessibility requirements. Current tests are missing:
- Loading state rendering (spinner display)
- Error state rendering (alert display)
- Primary button click behavior
- Close button (X) click behavior
- Primary button disabled state during loading
- Custom
primaryTextandsecondaryTextrendering - Behavior when
onPrimaryis undefined (primary button should not render)
Based on learnings, test isolation requires explicit per-file cleanup.
🔎 Proposed test additions
+import { afterEach } from 'vitest';
+
describe('CRUDModalTemplate', () => {
+ afterEach(() => {
+ vi.clearAllMocks();
+ });
+
it('renders title and content when open', () => {
// ... existing test
});
+ it('does not render when closed', () => {
+ render(
+ <CRUDModalTemplate
+ open={false}
+ title="Create Item"
+ onClose={() => {}}
+ >
+ <div>Modal Content</div>
+ </CRUDModalTemplate>
+ );
+
+ expect(screen.queryByText(/create item/i)).not.toBeInTheDocument();
+ });
+
+ it('displays loading spinner when loading', () => {
+ render(
+ <CRUDModalTemplate
+ open
+ title="Create Item"
+ onClose={() => {}}
+ loading
+ >
+ <div>Modal Content</div>
+ </CRUDModalTemplate>
+ );
+
+ expect(screen.getByRole('status')).toBeInTheDocument();
+ expect(screen.queryByText(/modal content/i)).not.toBeInTheDocument();
+ });
+
+ it('displays error message when error prop is provided', () => {
+ render(
+ <CRUDModalTemplate
+ open
+ title="Create Item"
+ onClose={() => {}}
+ error="Something went wrong"
+ />
+ );
+
+ expect(screen.getByText(/something went wrong/i)).toBeInTheDocument();
+ });
+
+ it('calls onPrimary when primary button is clicked', async () => {
+ const onPrimary = vi.fn();
+
+ render(
+ <CRUDModalTemplate
+ open
+ title="Create Item"
+ onClose={() => {}}
+ onPrimary={onPrimary}
+ />
+ );
+
+ await userEvent.click(screen.getByRole('button', { name: /save/i }));
+ expect(onPrimary).toHaveBeenCalledTimes(1);
+ });
+
+ it('disables primary button during loading', () => {
+ render(
+ <CRUDModalTemplate
+ open
+ title="Create Item"
+ onClose={() => {}}
+ onPrimary={() => {}}
+ loading
+ />
+ );
+
+ expect(screen.getByRole('button', { name: /save/i })).toBeDisabled();
+ });
+
+ it('does not render primary button when onPrimary is undefined', () => {
+ render(
+ <CRUDModalTemplate
+ open
+ title="Create Item"
+ onClose={() => {}}
+ />
+ );
+
+ expect(screen.queryByRole('button', { name: /save/i })).not.toBeInTheDocument();
+ expect(screen.getByRole('button', { name: /cancel/i })).toBeInTheDocument();
+ });
+
+ it('renders custom button text', () => {
+ render(
+ <CRUDModalTemplate
+ open
+ title="Create Item"
+ onClose={() => {}}
+ onPrimary={() => {}}
+ primaryText="Create"
+ secondaryText="Close"
+ />
+ );
+
+ expect(screen.getByRole('button', { name: /create/i })).toBeInTheDocument();
+ expect(screen.getByRole('button', { name: /close/i })).toBeInTheDocument();
+ });
});🤖 Prompt for AI Agents
In src/shared-components/CRUDModalTemplate/CRUDModalTemplate.spec.tsx around
lines 7 to 53, add per-file cleanup and expand tests: include afterEach(() => {
vi.clearAllMocks(); }) to reset mocks between tests; add tests to assert loading
state shows a spinner and disables/hides primary button, error state shows an
alert, clicking primary button invokes onPrimary, clicking close (X) invokes
onClose, custom primaryText/secondaryText are rendered, and verify primary
button is not rendered when onPrimary is undefined; ensure each new test renders
the component with the appropriate props and asserts DOM and callback behavior
to meet >90% coverage and accessibility checks.
Comment on lines
+1
to
+8
| import React, { useEffect } from 'react'; | ||
| import { Modal, Button, Spinner, Alert } from 'react-bootstrap'; | ||
| import type { CrudModalBaseProps } from './types'; | ||
|
|
||
| export interface CRUDModalTemplateProps extends CrudModalBaseProps { | ||
| children?: React.ReactNode; | ||
| onPrimary?: () => void; | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add component-level TSDoc.
Per project standards, shared components must include a brief TSDoc header describing the component's purpose and props. Add a JSDoc comment above the interface definition.
🔎 Proposed addition
+/**
+ * Base props interface for CRUD modal templates.
+ * Provides foundational properties for modal visibility, content, actions, and state.
+ */
export interface CRUDModalTemplateProps extends CrudModalBaseProps {
children?: React.ReactNode;
onPrimary?: () => void;
}📝 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.
Suggested change
| import React, { useEffect } from 'react'; | |
| import { Modal, Button, Spinner, Alert } from 'react-bootstrap'; | |
| import type { CrudModalBaseProps } from './types'; | |
| export interface CRUDModalTemplateProps extends CrudModalBaseProps { | |
| children?: React.ReactNode; | |
| onPrimary?: () => void; | |
| } | |
| import React, { useEffect } from 'react'; | |
| import { Modal, Button, Spinner, Alert } from 'react-bootstrap'; | |
| import type { CrudModalBaseProps } from './types'; | |
| /** | |
| * Base props interface for CRUD modal templates. | |
| * Provides foundational properties for modal visibility, content, actions, and state. | |
| */ | |
| export interface CRUDModalTemplateProps extends CrudModalBaseProps { | |
| children?: React.ReactNode; | |
| onPrimary?: () => void; | |
| } |
🤖 Prompt for AI Agents
In src/shared-components/CRUDModalTemplate/CRUDModalTemplate.tsx around lines 1
to 8, there is no component-level TSDoc for the CRUDModalTemplateProps
interface; add a JSDoc/TSDoc comment immediately above the interface that
briefly describes the component's purpose, summarizes the props (children and
onPrimary plus inherited CrudModalBaseProps), and any important behavior or
usage notes to comply with project standards.
Comment on lines
+22
to
+33
| useEffect(() => { | ||
| if (!open || !onPrimary) return; | ||
|
|
||
| const handleKeyDown = (e: KeyboardEvent) => { | ||
| if (e.key === 'Enter') { | ||
| onPrimary(); | ||
| } | ||
| }; | ||
|
|
||
| document.addEventListener('keydown', handleKeyDown); | ||
| return () => document.removeEventListener('keydown', handleKeyDown); | ||
| }, [open, onPrimary]); |
Contributor
There was a problem hiding this comment.
Critical: Enter key handler triggers on ALL keydown events.
The global keydown listener will fire onPrimary even when users are typing in form inputs, textareas, or contenteditable elements within the modal. This breaks standard form UX where Enter should add a newline in textareas or submit only when focused on non-text inputs.
🔎 Proposed fix to check event target
useEffect(() => {
if (!open || !onPrimary) return;
const handleKeyDown = (e: KeyboardEvent) => {
- if (e.key === 'Enter') {
+ const target = e.target as HTMLElement;
+ const isFormElement = target.matches('input, textarea, [contenteditable="true"]');
+
+ // Allow Enter in textareas and contenteditable; prevent in single-line inputs unless they're buttons/submits
+ if (e.key === 'Enter' && !isFormElement) {
onPrimary();
}
};
document.addEventListener('keydown', handleKeyDown);
return () => document.removeEventListener('keydown', handleKeyDown);
}, [open, onPrimary]);Alternatively, consider using a more refined approach that allows Enter on single-line inputs but not textareas:
const handleKeyDown = (e: KeyboardEvent) => {
const target = e.target as HTMLElement;
const tagName = target.tagName.toLowerCase();
// Block Enter in textareas and contenteditable
if (tagName === 'textarea' || target.isContentEditable) {
return;
}
// Allow Enter to trigger primary action
if (e.key === 'Enter') {
e.preventDefault();
onPrimary();
}
};📝 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.
Suggested change
| useEffect(() => { | |
| if (!open || !onPrimary) return; | |
| const handleKeyDown = (e: KeyboardEvent) => { | |
| if (e.key === 'Enter') { | |
| onPrimary(); | |
| } | |
| }; | |
| document.addEventListener('keydown', handleKeyDown); | |
| return () => document.removeEventListener('keydown', handleKeyDown); | |
| }, [open, onPrimary]); | |
| useEffect(() => { | |
| if (!open || !onPrimary) return; | |
| const handleKeyDown = (e: KeyboardEvent) => { | |
| const target = e.target as HTMLElement; | |
| const isFormElement = target.matches('input, textarea, [contenteditable="true"]'); | |
| // Allow Enter in textareas and contenteditable; prevent in single-line inputs unless they're buttons/submits | |
| if (e.key === 'Enter' && !isFormElement) { | |
| onPrimary(); | |
| } | |
| }; | |
| document.addEventListener('keydown', handleKeyDown); | |
| return () => document.removeEventListener('keydown', handleKeyDown); | |
| }, [open, onPrimary]); |
🤖 Prompt for AI Agents
In src/shared-components/CRUDModalTemplate/CRUDModalTemplate.tsx around lines 22
to 33, the global keydown listener currently triggers onPrimary for every Enter
press (including when typing in inputs, textareas, or contenteditable elements);
update the handler to inspect e.target and only call onPrimary for
non-text-entry elements: cast e.target to HTMLElement, check tagName (block if
'textarea') and check target.isContentEditable, and optionally block when the
target is an input of types that accept Enter (e.g., avoid triggering for inputs
where Enter should insert/submit inline); when triggering onPrimary call
e.preventDefault(); keep adding/removing the listener as-is.
Comment on lines
+1
to
+22
| export interface CrudModalBaseProps { | ||
| /** Controls modal visibility */ | ||
| open: boolean; | ||
|
|
||
| /** Modal title */ | ||
| title: string; | ||
|
|
||
| /** Close handler (Cancel / X / Esc) */ | ||
| onClose: () => void; | ||
|
|
||
| /** Primary button text */ | ||
| primaryText?: string; | ||
|
|
||
| /** Secondary button text */ | ||
| secondaryText?: string; | ||
|
|
||
| /** Loading state */ | ||
| loading?: boolean; | ||
|
|
||
| /** Error message */ | ||
| error?: string; | ||
| } |
Contributor
There was a problem hiding this comment.
Move type definitions to standard location.
Per project conventions, shared component prop interfaces must be defined in src/types/CRUDModalTemplate/interface.ts rather than co-located with the component. This maintains consistency with other shared components in the codebase.
Based on learnings, the refactoring standard requires: "all props in src/types//interface.ts with brief TSDoc."
🔎 Proposed file relocation
- Create
src/types/CRUDModalTemplate/interface.tswith this content:
export interface CrudModalBaseProps {
/** Controls modal visibility */
open: boolean;
/** Modal title */
title: string;
/** Close handler (Cancel / X / Esc) */
onClose: () => void;
/** Primary button text */
primaryText?: string;
/** Secondary button text */
secondaryText?: string;
/** Loading state */
loading?: boolean;
/** Error message */
error?: string;
}- Update imports in
CRUDModalTemplate.tsx:
-import type { CrudModalBaseProps } from './types';
+import type { CrudModalBaseProps } from '../../types/CRUDModalTemplate/interface';- Update re-export in
index.ts:
-export * from './types';
+export * from '../../types/CRUDModalTemplate/interface';- Delete
src/shared-components/CRUDModalTemplate/types.ts
🤖 Prompt for AI Agents
In src/shared-components/CRUDModalTemplate/types.ts around lines 1 to 22, move
the CrudModalBaseProps interface to the canonical location
src/types/CRUDModalTemplate/interface.ts by creating that file with the same
interface and brief TSDoc comments for each prop; update any imports in the
CRUDModalTemplate component file(s) to import CrudModalBaseProps from
"src/types/CRUDModalTemplate/interface" and update the package's index
re-exports to point to the new file if applicable; then remove the original
src/shared-components/CRUDModalTemplate/types.ts file to avoid duplication and
ensure the codebase follows the project convention.
Contributor
|
Are you still working on this? |
Author
Yes @palisadoes I'm still working on this, my health isn't good so need some time but yeah I will finish it asap |
Contributor
|
25 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What kind of change does this PR introduce?
Feature / Foundation Component
Issue Number
Fixes #5292
Snapshots / Videos
N/A (foundational shared component, no UI integration yet).
A short video showing the added/changed files is attached.
If relevant, did you update the documentation?
Not applicable for this foundational component.
Summary
This PR introduces the foundational
CRUDModalTemplateshared component.This PR only covers Phase 1 (foundation). Phase 2 will be submitted separately.
Does this PR introduce a breaking change?
No. This is an additive, non-breaking change.
Checklist
CodeRabbit AI Review
Test Coverage
Other information
N/A
Have you read the contributing guide?
Yes
https://github.com/user-attachments/assets/0c1e4db2-94f2-4ab4-88b4-091baf2497af
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.