refactor(ActionItemModal): extract AssignmentTypeSelector component [Part 2/5] #6383
Conversation
|
To generate Unit Tests for this PR, please click here. |
Our Pull Request Approval ProcessThis PR will be reviewed according to our: Your PR may be automatically closed if:
Thanks for contributing! |
WalkthroughExtracts assignment type selection into a new AssignmentTypeSelector component, integrates it into ActionItemModal replacing inline UI, strengthens typing and validation, adds test coverage for the selector, and updates ApplyToSelector to use React's useId for dynamic radio IDs. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 inconclusive)
✅ Passed checks (6 passed)
✨ Finishing touches
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 |
nishantharkut
force-pushed
the
refactor/ActionItemModal-part2
branch
from
0c0df9b to
c0dac8c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
@src/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.spec.tsx:
- Around line 72-118: Add explicit keyboard-interaction tests for the
interactive chips: create separate test cases (e.g., it('calls onTypeChange with
volunteer when Enter key is pressed on volunteer chip') and corresponding tests
for Space and for the volunteerGroup chip) that render via renderComponent,
query the chip using screen.getByText('Volunteer').closest('[role="button"]') or
'Volunteer Group', call .focus() on the returned
volunteerChip/volunteerGroupChip, then simulate keyboard activation with
userEvent.setup() followed by await user.keyboard('{Enter}') and await
user.keyboard('{Space}') as separate tests, and assert onTypeChange was called
with the correct value ('volunteer' or 'volunteerGroup') and that
onClearVolunteer/onClearVolunteerGroup were invoked where appropriate.
In
@src/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.tsx:
- Around line 11-23: The component TSDoc for AssignmentTypeSelector lacks flat
@param tags for its props; update the comment block above the
AssignmentTypeSelector declaration to include @param entries for each prop
(assignmentType, onTypeChange, isVolunteerDisabled, isVolunteerGroupDisabled,
onClearVolunteer, onClearVolunteerGroup), using the flat parameter documentation
style used in TSX files so each prop has a short description and type/context in
the TSDoc.
- Around line 28-29: Remove the unused groupId created by useId(): delete the
const groupId = useId() declaration and remove id={groupId} from the Box element
in AssignmentTypeSelector so no dead variable or unused id prop remains; keep
labelId if still used for the label association.
- Around line 50-55: Replace the non-semantic <Box> wrapper in
AssignmentTypeSelector with a semantic fieldset to improve accessibility: use
the existing Box but set component="fieldset" (or replace with a native
<fieldset>) instead of relying on role="group", keep id={groupId} and className
for styling, remove role="group", and ensure the aria-labelledby still points to
labelId (or add a <legend> using labelId) so the group remains properly
labelled.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (4)
docs/docs/auto-docs/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector/variables/default.mdis excluded by!**/docs/docs/**docs/docs/auto-docs/screens/OrganizationActionItems/ActionItemModal/ActionItemModal/variables/default.mdis excluded by!**/docs/docs/**docs/docs/auto-docs/types/AdminPortal/AssignmentTypeSelector/interface/interfaces/InterfaceAssignmentTypeSelectorProps.mdis excluded by!**/docs/docs/**docs/docs/auto-docs/types/AdminPortal/AssignmentTypeSelector/interface/type-aliases/AssignmentType.mdis excluded by!**/docs/docs/**
📒 Files selected for processing (6)
src/components/AdminPortal/ApplyToSelector/ApplyToSelector.tsxsrc/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.spec.tsxsrc/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.tsxsrc/screens/OrganizationActionItems/ActionItemModal/ActionItemModal.spec.tsxsrc/screens/OrganizationActionItems/ActionItemModal/ActionItemModal.tsxsrc/types/AdminPortal/AssignmentTypeSelector/interface.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (Custom checks)
Test coverage must be ≥95% for modified files. Run: pnpm run test:coverage and verify coverage/coverage-summary.json shows no files below threshold
Files:
src/screens/OrganizationActionItems/ActionItemModal/ActionItemModal.spec.tsxsrc/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.spec.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (Custom checks)
Zero TypeScript errors: Run pnpm run typecheck and must pass without errors or warnings
Files:
src/screens/OrganizationActionItems/ActionItemModal/ActionItemModal.spec.tsxsrc/types/AdminPortal/AssignmentTypeSelector/interface.tssrc/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.tsxsrc/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.spec.tsxsrc/components/AdminPortal/ApplyToSelector/ApplyToSelector.tsxsrc/screens/OrganizationActionItems/ActionItemModal/ActionItemModal.tsx
**/*
⚙️ CodeRabbit configuration file
**/*: Conduct a comprehensive review and reevaluate all resolved and dismissed items.VALIDATION FRAMEWORK
1. Test Coverage & Quality
1.1 Test File Requirements
For each modified source file with logic or UI:
- ✅ Test file exists:
*.spec.tsx,*.spec.ts,*.test.tsx, or*.test.ts- ✅ Test file colocated with component (same directory)
- ✅ Test file updated to reflect source changes
- ✅ Target ≥95% code coverage for new/modified code
- ✅ List specific line numbers lacking test coverage
1.2 Test Quality Standards
✅ Framework Compliance (Vitest + React Testing Library):
- Use
vi.mock()for mocking (notjest.mock())- Use
describe(),it(),test()for structure- Use
@testing-library/reactqueries (getByRole, getByText, etc.)- Use
@testing-library/user-eventfor interactions✅ Coverage Completeness - All scenarios tested:
- Success paths with valid data
- Error scenarios (network failures, GraphQL errors, validation errors)
- Edge cases (empty data, null/undefined, boundary values)
- Loading and pending states
- User interactions and state changes
✅ Mock Quality:
- GraphQL:
MockedProviderorStaticMockLink- Realistic, comprehensive mock data
- Proper cleanup (reset mocks, clear timers)
✅ Test Structure:
- Isolated tests (no execution order dependency)
- No disabled tests (
*.skip) without documented reason + linked issue- Proper async utilities (
waitFor,act)- Shard-compatible (12-shard strategy, no shared state, <30s timeout)
2. Code Quality & Implementation
2.1 TypeScript Standards
- ✅ No
anytype without JSDoc justification- ✅ Proper typing for:
- Component props (interface declarations)
- Function parameters and return types
- GraphQL variables and responses
- State and context
- ✅ No
@ts-ignorewithout explanation- ✅ Zero TypeScript compilation errors
2.2 React & GraphQL Implementation
- ✅ React Best Practices:
- Fu...
Files:
src/screens/OrganizationActionItems/ActionItemModal/ActionItemModal.spec.tsxsrc/types/AdminPortal/AssignmentTypeSelector/interface.tssrc/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.tsxsrc/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.spec.tsxsrc/components/AdminPortal/ApplyToSelector/ApplyToSelector.tsxsrc/screens/OrganizationActionItems/ActionItemModal/ActionItemModal.tsx
src/**/components/**/*.{ts,tsx}
📄 CodeRabbit inference engine (Custom checks)
All components must follow reusable component policy: No inline interfaces, correct portal placement, wrapper usage
Files:
src/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.tsxsrc/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.spec.tsxsrc/components/AdminPortal/ApplyToSelector/ApplyToSelector.tsx
🧠 Learnings (10)
📚 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/screens/OrganizationActionItems/ActionItemModal/ActionItemModal.spec.tsxsrc/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.spec.tsx
📚 Learning: 2025-12-27T11:22:41.191Z
Learnt from: rawadhossain
Repo: PalisadoesFoundation/talawa-admin PR: 5782
File: src/screens/Leaderboard/Leaderboard.spec.tsx:342-370
Timestamp: 2025-12-27T11:22:41.191Z
Learning: In test files that render screens with shared components (e.g., src/screens/Leaderboard/Leaderboard.spec.tsx), verify integration behavior by asserting the screen renders correctly in different scenarios (e.g., search results) without asserting the internal ARIA attributes, text, or icons of the shared component. Rely on the shared component's own tests to cover accessibility and presentation details. This keeps tests focused on higher-level behavior and interactions.
Applied to files:
src/screens/OrganizationActionItems/ActionItemModal/ActionItemModal.spec.tsxsrc/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.spec.tsx
📚 Learning: 2026-01-08T18:48:10.364Z
Learnt from: abhishekblue
Repo: PalisadoesFoundation/talawa-admin PR: 6260
File: src/screens/AdminPortal/OrganizationPeople/OrganizationPeople.spec.tsx:337-339
Timestamp: 2026-01-08T18:48:10.364Z
Learning: In tests that exercise Apollo Client queries (e.g., talawa-admin tests under src/screens/.../OrganizationPeople.spec.tsx), avoid using vi.useFakeTimers() because Apollo's internal promise resolution can be disrupted by mocked timers, causing timeouts. Use real timers with explicit waits (for example, await act(async () => { await new Promise(r => setTimeout(r, DURATION)); })) when testing components that fetch data via Apollo. If you must use timers, prefer real timers for the relevant test and flush promises deterministically. Replace or remove useFakeTimers in such tests and rely on actual time passage or proper promise flushing to ensure reliable Apollo-driven data fetching tests.
Applied to files:
src/screens/OrganizationActionItems/ActionItemModal/ActionItemModal.spec.tsxsrc/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.spec.tsx
📚 Learning: 2025-12-26T20:34:53.912Z
Learnt from: MuhammadUmar7195
Repo: PalisadoesFoundation/talawa-admin PR: 5847
File: src/screens/UserPortal/Volunteer/Invitations/Invitations.tsx:243-243
Timestamp: 2025-12-26T20:34:53.912Z
Learning: In PalisadoesFoundation/talawa-admin, whenever you modify a file (e.g., for migrations, tests, or feature changes), replace inline color attributes like color="grey" with Bootstrap utility classes (e.g., className="text-secondary") to satisfy the CSS-Policy-Check CI. This is a general compliance guideline, not scope creep, and should be applied to all affected TSX files during reviews. Steps: 1) Find inline color attributes in JSX/TSX. 2) Replace with appropriate Bootstrap text/color utility classes. 3) Ensure the visual outcome remains the same. 4) Run CI checks to confirm the policy passes.
Applied to files:
src/screens/OrganizationActionItems/ActionItemModal/ActionItemModal.spec.tsxsrc/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.tsxsrc/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.spec.tsxsrc/components/AdminPortal/ApplyToSelector/ApplyToSelector.tsxsrc/screens/OrganizationActionItems/ActionItemModal/ActionItemModal.tsx
📚 Learning: 2026-01-06T16:39:41.033Z
Learnt from: Jashan32
Repo: PalisadoesFoundation/talawa-admin PR: 6199
File: src/shared-components/ProfileForm/ProfileForm.tsx:0-0
Timestamp: 2026-01-06T16:39:41.033Z
Learning: When refactoring or consolidating components (e.g., talawa-admin), remove helper functions that were copied from old files but are not used in the new implementation. Before exporting utilities, verify there are real usages across the codebase (search usages, remove dead code, and keep only necessary exports) to improve maintainability and reduce bundle size.
Applied to files:
src/screens/OrganizationActionItems/ActionItemModal/ActionItemModal.spec.tsxsrc/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.tsxsrc/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.spec.tsxsrc/components/AdminPortal/ApplyToSelector/ApplyToSelector.tsxsrc/screens/OrganizationActionItems/ActionItemModal/ActionItemModal.tsx
📚 Learning: 2026-01-07T09:08:09.150Z
Learnt from: adityai0
Repo: PalisadoesFoundation/talawa-admin PR: 6202
File: src/components/EventManagement/EventAgendaItems/EventAgendaItems.tsx:4-13
Timestamp: 2026-01-07T09:08:09.150Z
Learning: In the talawa-admin repository, document comments with TSDoc in all TSX files. Do not use JSDoc. Write non-standard tags like component and requires as plain text (no leading @). Use standard TSDoc tags such as param, remarks, returns, and example with the leading @. This guideline applies to all TSX components (e.g., src/components/.../*.tsx).
Applied to files:
src/screens/OrganizationActionItems/ActionItemModal/ActionItemModal.spec.tsxsrc/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.tsxsrc/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.spec.tsxsrc/components/AdminPortal/ApplyToSelector/ApplyToSelector.tsxsrc/screens/OrganizationActionItems/ActionItemModal/ActionItemModal.tsx
📚 Learning: 2025-12-27T11:22:19.556Z
Learnt from: rawadhossain
Repo: PalisadoesFoundation/talawa-admin PR: 5782
File: src/screens/Leaderboard/Leaderboard.spec.tsx:299-329
Timestamp: 2025-12-27T11:22:19.556Z
Learning: In talawa-admin test files (e.g., src/screens/Leaderboard/Leaderboard.spec.tsx), prefer separate explicit test cases for keyboard navigation interactions (Enter key, Space key) instead of consolidating them with test.each()/parameterization. This improves readability, documents each keyboard interaction path clearly, and makes failures easier to diagnose when specific key handling regresses. Apply this pattern to similarly structured spec files by keeping individual tests for each key interaction rather than combining them into a single parameterized test.
Applied to files:
src/screens/OrganizationActionItems/ActionItemModal/ActionItemModal.spec.tsxsrc/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.spec.tsx
📚 Learning: 2026-01-06T21:27:50.880Z
Learnt from: Jashan32
Repo: PalisadoesFoundation/talawa-admin PR: 6222
File: src/utils/userUpdateUtils.ts:22-22
Timestamp: 2026-01-06T21:27:50.880Z
Learning: In the Talawa Admin codebase, the i18n extraction tool can misclassify TypeScript generic syntax (e.g., (obj: T): Partial) as translatable content. Suppress these false positives by adding a // i18n-ignore-next-line comment directly above the line with the type declaration or generic function signature. Apply this only to TS code where misclassification is confirmed; avoid blanket suppression.
Applied to files:
src/types/AdminPortal/AssignmentTypeSelector/interface.ts
📚 Learning: 2026-01-07T14:21:19.711Z
Learnt from: cotishq
Repo: PalisadoesFoundation/talawa-admin PR: 6198
File: src/setup/setup.ts:123-124
Timestamp: 2026-01-07T14:21:19.711Z
Learning: Use TSDoc (eslint-plugin-tsdoc) instead of JSDoc. Do not wrap type names in curly braces in throws and returns. Write as "throws Error" and "returns Promise<void>" (no braces). Curly-brace usage causes tsdoc-malformed-inline-tag and tsdoc-escape-right-brace lint errors. Apply this rule to all TypeScript source files (e.g., src/**/*.ts) to ensure consistency.
Applied to files:
src/types/AdminPortal/AssignmentTypeSelector/interface.ts
📚 Learning: 2026-01-08T16:45:49.201Z
Learnt from: adityai0
Repo: PalisadoesFoundation/talawa-admin PR: 6241
File: src/components/AdminPortal/OrgPeopleListCard/OrgPeopleListCard.tsx:12-16
Timestamp: 2026-01-08T16:45:49.201Z
Learning: In TSX components, prefer flat JSDoc parameter documentation format (e.g., param id, param toggleRemoveModal) over the nested format (param props.id). The nested form can break editor parsing. When a component accepts a props object, destructure the props in the function signature to align with the flat JSDoc style (e.g., function MyComp({ id, toggleRemoveModal }: Props) { ... }). Ensure all param tags correspond to the destructured props and reflect actual component props to improve editor support and readability.
Applied to files:
src/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.tsxsrc/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.spec.tsxsrc/components/AdminPortal/ApplyToSelector/ApplyToSelector.tsx
🧬 Code graph analysis (3)
src/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.tsx (1)
src/types/AdminPortal/AssignmentTypeSelector/interface.ts (1)
InterfaceAssignmentTypeSelectorProps(11-24)
src/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.spec.tsx (1)
src/types/AdminPortal/AssignmentTypeSelector/interface.ts (1)
AssignmentType(6-6)
src/screens/OrganizationActionItems/ActionItemModal/ActionItemModal.tsx (5)
src/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.tsx (1)
AssignmentType(9-9)src/types/AdminPortal/AssignmentTypeSelector/interface.ts (1)
AssignmentType(6-6)src/utils/interfaces.ts (1)
InterfaceEventVolunteerInfo(2540-2571)src/types/shared-components/ActionItems/interface.ts (2)
IEventVolunteerGroup(171-201)IFormStateType(121-130)src/components/NotificationToast/NotificationToast.tsx (1)
NotificationToast(76-82)
🪛 Biome (2.1.2)
src/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.tsx
[error] 52-52: The elements with this role can be changed to the following elements:
For examples and more information, see WAI-ARIA Roles
(lint/a11y/useSemanticElements)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Performs linting, formatting, type-checking, unused file detection, checking for different source...
- GitHub Check: Translation Tag Check
- GitHub Check: Analyse Code With CodeQL (javascript)
🔇 Additional comments (16)
src/components/AdminPortal/ApplyToSelector/ApplyToSelector.tsx (1)
1-53: LGTM!The addition of dynamic ID generation using
useId()is a solid accessibility improvement. This ensures unique IDs for radio inputs when multiple instances of the component are rendered, preventing ID conflicts and maintaining proper label-input associations. The// i18n-ignore-linecomments appropriately suppress false positives from the i18n extraction tool.src/screens/OrganizationActionItems/ActionItemModal/ActionItemModal.spec.tsx (3)
17-25: LGTM!The updated imports and top-level
afterEachhook withvi.clearAllMocks()ensure proper test isolation. This follows the learned pattern for per-file mock hygiene in talawa-admin test files.Also applies to: 61-63
970-970: Good fix: Remove async from describe callbacks.Removing the
asynckeyword from the describe block callbacks is correct. Vitest/Jest'sdescribefunction doesn't handle async callbacks, and havingasyncthere could lead to unexpected behavior since the returned promise would be ignored.Also applies to: 1228-1228
1635-1638: LGTM!The
afterAllhook properly clears mocks and timers at the end of the test suite, ensuring clean state for subsequent test files.src/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.spec.tsx (1)
1-185: Good test coverage for the new component.The test suite comprehensively covers:
- Rendering both chip options
- Selection state based on
assignmentTypeprop- Click handler invocations with correct arguments
- Clearing opposite selections when switching types
- Disabled chip behavior (no handler calls)
- Disabled state styling via
aria-disabledThe
afterEachwithvi.clearAllMocks()ensures test isolation per the project's learnings.src/types/AdminPortal/AssignmentTypeSelector/interface.ts (1)
1-24: LGTM!The type definitions follow the project's reusable component architecture guidelines:
- Interface placed in
src/types/AdminPortal/AssignmentTypeSelector/interface.ts(correct directory)- Naming follows
InterfaceAssignmentTypeSelectorPropsconvention- TSDoc comments properly document the type and each property
AssignmentTypeunion provides clear type safety for assignment modessrc/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.tsx (1)
1-85: Overall implementation is solid.The component correctly:
- Imports props interface from the designated types directory
- Re-exports
AssignmentTypefor consumer convenience- Uses
useId()for unique accessibility IDs- Handles disabled states with both visual feedback and click prevention
- Uses translation keys for i18n compliance
src/screens/OrganizationActionItems/ActionItemModal/ActionItemModal.tsx (9)
7-7: LGTM on import updates.The imports are properly updated:
useCallbackadded for memoizinghandleFormChangeAssignmentTypeSelectorandAssignmentTypeimported from correct paths- Unused MUI imports (
Chip,Box,Typography) removed since the inline UI is now handled byAssignmentTypeSelectorAlso applies to: 17-18, 40-40
85-86: LGTM!The
assignmentTypestate is now properly typed using theAssignmentTypeunion from the centralized interface file, improving type safety.
130-133: Good centralization of recurring event logic.The
isApplyToRelevantcomputed value cleanly encapsulates when the "applyTo" (series vs. instance) selection matters:
- Must be a recurring event
- Either creating new (not editMode) or editing a template that isn't an instance exception
This prevents the filtering logic from running unnecessarily for non-recurring scenarios.
194-202: LGTM!Wrapping
handleFormChangeinuseCallbackwith an empty dependency array is correct since the function only usessetFormState(which is stable). This prevents unnecessary re-creations and allows safe inclusion in theuseEffectdependency array on line 446.
215-229: Improved validation guards.These additional validation checks are good defensive programming:
- Lines 215-221: Ensures
baseEvent.idexists when creating for series in recurring events- Lines 223-229: Ensures
eventIdexists for non-recurring event creationBoth provide early returns with appropriate error toasts rather than letting the mutation fail with confusing errors.
323-329: Good validation addition.The explicit check for
eventIdbefore callingupdateActionForInstanceprevents potential runtime errors. The mutation requireseventIdin its input, so this guard ensures the invariant is enforced early with a clear error message.
378-387: Review the applyTo initialization logic.The updated conditions handle three scenarios:
isInstanceException→ set to 'instance'isTemplate(and not instance exception) → set to 'series'- Existing actionItem (non-template, non-exception) → set to 'instance'
This is correct. However, note that the effect runs whenever
actionItemchanges. For new action items (actionItemis null), the separate effect on lines 450-454 handles the reset to 'instance'.
421-447: Good addition ofisApplyToRelevantguard.The early return at line 423 prevents unnecessary state updates when the applyTo selection isn't relevant (non-recurring events or editing non-template items). The dependency array correctly includes all referenced values including the memoized
handleFormChange.
506-519: Successful extraction to AssignmentTypeSelector.The inline Chip-based UI is cleanly replaced with the extracted
AssignmentTypeSelectorcomponent. The props mapping is correct:
assignmentTypeandonTypeChangefor state management- Disabled flags from the computed values
- Clear callbacks properly reset both form state and selected object state
This reduces code duplication and improves maintainability as per the PR objectives.
| it('calls onTypeChange with volunteer when volunteer chip is clicked', async () => { | ||
| const onTypeChange = vi.fn(); | ||
| const onClearVolunteerGroup = vi.fn(); | ||
| renderComponent( | ||
| 'volunteerGroup', | ||
| onTypeChange, | ||
| false, | ||
| false, | ||
| vi.fn(), | ||
| onClearVolunteerGroup, | ||
| ); | ||
|
|
||
| const user = userEvent.setup(); | ||
| const volunteerChip = screen | ||
| .getByText('Volunteer') | ||
| .closest('[role="button"]'); | ||
| if (volunteerChip) { | ||
| await user.click(volunteerChip); | ||
| } | ||
|
|
||
| expect(onTypeChange).toHaveBeenCalledWith('volunteer'); | ||
| expect(onClearVolunteerGroup).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('calls onTypeChange with volunteerGroup when volunteer group chip is clicked', async () => { | ||
| const onTypeChange = vi.fn(); | ||
| const onClearVolunteer = vi.fn(); | ||
| renderComponent( | ||
| 'volunteer', | ||
| onTypeChange, | ||
| false, | ||
| false, | ||
| onClearVolunteer, | ||
| vi.fn(), | ||
| ); | ||
|
|
||
| const user = userEvent.setup(); | ||
| const volunteerGroupChip = screen | ||
| .getByText('Volunteer Group') | ||
| .closest('[role="button"]'); | ||
| if (volunteerGroupChip) { | ||
| await user.click(volunteerGroupChip); | ||
| } | ||
|
|
||
| expect(onTypeChange).toHaveBeenCalledWith('volunteerGroup'); | ||
| expect(onClearVolunteer).toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding keyboard navigation tests.
Based on learnings, the project prefers separate explicit test cases for keyboard interactions (Enter key, Space key). Since these Chips are interactive elements, consider adding tests for keyboard activation:
it('calls onTypeChange with volunteer when Enter key is pressed on volunteer chip', async () => {
const onTypeChange = vi.fn();
const onClearVolunteerGroup = vi.fn();
renderComponent('volunteerGroup', onTypeChange, false, false, vi.fn(), onClearVolunteerGroup);
const user = userEvent.setup();
const volunteerChip = screen.getByText('Volunteer').closest('[role="button"]');
if (volunteerChip) {
volunteerChip.focus();
await user.keyboard('{Enter}');
}
expect(onTypeChange).toHaveBeenCalledWith('volunteer');
});🤖 Prompt for AI Agents
In
@src/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.spec.tsx
around lines 72 - 118, Add explicit keyboard-interaction tests for the
interactive chips: create separate test cases (e.g., it('calls onTypeChange with
volunteer when Enter key is pressed on volunteer chip') and corresponding tests
for Space and for the volunteerGroup chip) that render via renderComponent,
query the chip using screen.getByText('Volunteer').closest('[role="button"]') or
'Volunteer Group', call .focus() on the returned
volunteerChip/volunteerGroupChip, then simulate keyboard activation with
userEvent.setup() followed by await user.keyboard('{Enter}') and await
user.keyboard('{Space}') as separate tests, and assert onTypeChange was called
with the correct value ('volunteer' or 'volunteerGroup') and that
onClearVolunteer/onClearVolunteerGroup were invoked where appropriate.
| /** | ||
| * Chip-based toggle selector for choosing assignment type (volunteer or volunteer group). | ||
| */ | ||
| const AssignmentTypeSelector: React.FC< | ||
| InterfaceAssignmentTypeSelectorProps | ||
| > = ({ | ||
| assignmentType, | ||
| onTypeChange, | ||
| isVolunteerDisabled, | ||
| isVolunteerGroupDisabled, | ||
| onClearVolunteer, | ||
| onClearVolunteerGroup, | ||
| }) => { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Enhance TSDoc with @param tags.
Per project learnings, TSDoc in TSX files should use flat parameter documentation format. Consider updating:
/**
* Chip-based toggle selector for choosing assignment type (volunteer or volunteer group).
+ *
+ * @param assignmentType - Current assignment type selection
+ * @param onTypeChange - Callback fired when assignment type changes
+ * @param isVolunteerDisabled - Whether volunteer chip is disabled
+ * @param isVolunteerGroupDisabled - Whether volunteer group chip is disabled
+ * @param onClearVolunteer - Callback to clear volunteer selection
+ * @param onClearVolunteerGroup - Callback to clear volunteer group selection
+ * @returns JSX.Element
*/🤖 Prompt for AI Agents
In @src/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.tsx
around lines 11 - 23, The component TSDoc for AssignmentTypeSelector lacks flat
@param tags for its props; update the comment block above the
AssignmentTypeSelector declaration to include @param entries for each prop
(assignmentType, onTypeChange, isVolunteerDisabled, isVolunteerGroupDisabled,
onClearVolunteer, onClearVolunteerGroup), using the flat parameter documentation
style used in TSX files so each prop has a short description and type/context in
the TSDoc.
| const labelId = useId(); | ||
| const groupId = useId(); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove unused groupId variable.
The groupId generated via useId() is assigned to the Box's id attribute but isn't referenced elsewhere (no aria-describedby or similar usage). Consider removing it to avoid dead code:
const labelId = useId();
- const groupId = useId();And remove id={groupId} from the Box on line 54.
🤖 Prompt for AI Agents
In @src/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.tsx
around lines 28 - 29, Remove the unused groupId created by useId(): delete the
const groupId = useId() declaration and remove id={groupId} from the Box element
in AssignmentTypeSelector so no dead variable or unused id prop remains; keep
labelId if still used for the label association.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/screens/OrganizationActionItems/ActionItemModal/ActionItemModal.tsx (1)
62-73: Add TSDoc to the exported component.Per coding guidelines, all exported components require TSDoc comments documenting the component's purpose, props, and return type.
📝 Add TSDoc
+/** + * Modal component for creating and updating action items. + * Handles assignment to volunteers or volunteer groups, category selection, + * and supports recurring event patterns with series/instance targeting. + * + * @param props - Component props from IItemModalProps + * @returns JSX.Element + */ const ItemModal: FC<IItemModalProps> = ({ isOpen, hide,
🤖 Fix all issues with AI agents
In
@src/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.tsx:
- Around line 11-13: Update the TSDoc for the exported AssignmentTypeSelector
component to include a @param for the props object and document each prop the
component accepts (e.g., selected value, onChange handler, optional
className/aria props or any others used inside AssignmentTypeSelector), include
the expected types/allowed values and behavior for each prop, note any
defaults/optional props, and add a @returns line describing the React element;
place this expanded docblock directly above the AssignmentTypeSelector export.
- Around line 50-55: Replace the non-semantic Box group with a semantic fieldset
by rendering Box with component="fieldset" in AssignmentTypeSelector; remove
role="group" (native fieldset provides grouping) but keep
aria-labelledby={labelId} and id={groupId}, and add layout-reset classes like
"border-0 p-0 m-0 d-flex gap-2" so the visual styles are preserved while using
the native <fieldset>.
In @src/screens/OrganizationActionItems/ActionItemModal/ActionItemModal.tsx:
- Around line 323-329: The validation currently logs a generic 'unknownError'
when eventId is missing in ActionItemModal (the if (!eventId) block); change the
NotificationToast.error call to use a specific i18n key (e.g. 'missingEventId'
or 'validation.eventIdRequired') and appropriate namespace so the toast maps to
a clear, localized validation message for the missing eventId; update any tests
or translation files that expect the old key accordingly.
- Around line 506-519: Extract the inline clear handlers passed to
AssignmentTypeSelector into memoized callbacks: create handleClearVolunteer and
handleClearVolunteerGroup using useCallback, each calling
handleFormChange('volunteerId','') / handleFormChange('volunteerGroupId','') and
setSelectedVolunteer(null) / setSelectedVolunteerGroup(null); include
handleFormChange in the dependency arrays (and any setters if lint requires) and
pass the memoized functions to AssignmentTypeSelector as onClearVolunteer and
onClearVolunteerGroup.
- Around line 215-229: Replace the generic unknownError toast with specific
translation keys for each validation branch so callers and users can distinguish
failures: when isRecurring && applyTo === 'series' && !baseEvent?.id, call
NotificationToast.error with a key like 'recurringSeriesMissingBaseEvent'
(namespace 'organizationActionItems') and when !isRecurring && !eventId, call
NotificationToast.error with a key like 'singleEventMissingId' (same namespace);
then add those keys and messages to all locale files under the
organizationActionItems namespace so translations exist for both cases.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (4)
docs/docs/auto-docs/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector/variables/default.mdis excluded by!**/docs/docs/**docs/docs/auto-docs/screens/OrganizationActionItems/ActionItemModal/ActionItemModal/variables/default.mdis excluded by!**/docs/docs/**docs/docs/auto-docs/types/AdminPortal/AssignmentTypeSelector/interface/interfaces/InterfaceAssignmentTypeSelectorProps.mdis excluded by!**/docs/docs/**docs/docs/auto-docs/types/AdminPortal/AssignmentTypeSelector/interface/type-aliases/AssignmentType.mdis excluded by!**/docs/docs/**
📒 Files selected for processing (6)
src/components/AdminPortal/ApplyToSelector/ApplyToSelector.tsxsrc/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.spec.tsxsrc/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.tsxsrc/screens/OrganizationActionItems/ActionItemModal/ActionItemModal.spec.tsxsrc/screens/OrganizationActionItems/ActionItemModal/ActionItemModal.tsxsrc/types/AdminPortal/AssignmentTypeSelector/interface.ts
🧰 Additional context used
📓 Path-based instructions (4)
src/**/components/**/*.{ts,tsx}
📄 CodeRabbit inference engine (Custom checks)
All components must follow reusable component policy: No inline interfaces, correct portal placement, wrapper usage
Files:
src/components/AdminPortal/ApplyToSelector/ApplyToSelector.tsxsrc/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.spec.tsxsrc/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (Custom checks)
Zero TypeScript errors: Run pnpm run typecheck and must pass without errors or warnings
Files:
src/components/AdminPortal/ApplyToSelector/ApplyToSelector.tsxsrc/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.spec.tsxsrc/types/AdminPortal/AssignmentTypeSelector/interface.tssrc/screens/OrganizationActionItems/ActionItemModal/ActionItemModal.spec.tsxsrc/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.tsxsrc/screens/OrganizationActionItems/ActionItemModal/ActionItemModal.tsx
**/*
⚙️ CodeRabbit configuration file
**/*: Conduct a comprehensive review and reevaluate all resolved and dismissed items.VALIDATION FRAMEWORK
1. Test Coverage & Quality
1.1 Test File Requirements
For each modified source file with logic or UI:
- ✅ Test file exists:
*.spec.tsx,*.spec.ts,*.test.tsx, or*.test.ts- ✅ Test file colocated with component (same directory)
- ✅ Test file updated to reflect source changes
- ✅ Target ≥95% code coverage for new/modified code
- ✅ List specific line numbers lacking test coverage
1.2 Test Quality Standards
✅ Framework Compliance (Vitest + React Testing Library):
- Use
vi.mock()for mocking (notjest.mock())- Use
describe(),it(),test()for structure- Use
@testing-library/reactqueries (getByRole, getByText, etc.)- Use
@testing-library/user-eventfor interactions✅ Coverage Completeness - All scenarios tested:
- Success paths with valid data
- Error scenarios (network failures, GraphQL errors, validation errors)
- Edge cases (empty data, null/undefined, boundary values)
- Loading and pending states
- User interactions and state changes
✅ Mock Quality:
- GraphQL:
MockedProviderorStaticMockLink- Realistic, comprehensive mock data
- Proper cleanup (reset mocks, clear timers)
✅ Test Structure:
- Isolated tests (no execution order dependency)
- No disabled tests (
*.skip) without documented reason + linked issue- Proper async utilities (
waitFor,act)- Shard-compatible (12-shard strategy, no shared state, <30s timeout)
2. Code Quality & Implementation
2.1 TypeScript Standards
- ✅ No
anytype without JSDoc justification- ✅ Proper typing for:
- Component props (interface declarations)
- Function parameters and return types
- GraphQL variables and responses
- State and context
- ✅ No
@ts-ignorewithout explanation- ✅ Zero TypeScript compilation errors
2.2 React & GraphQL Implementation
- ✅ React Best Practices:
- Fu...
Files:
src/components/AdminPortal/ApplyToSelector/ApplyToSelector.tsxsrc/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.spec.tsxsrc/types/AdminPortal/AssignmentTypeSelector/interface.tssrc/screens/OrganizationActionItems/ActionItemModal/ActionItemModal.spec.tsxsrc/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.tsxsrc/screens/OrganizationActionItems/ActionItemModal/ActionItemModal.tsx
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (Custom checks)
Test coverage must be ≥95% for modified files. Run: pnpm run test:coverage and verify coverage/coverage-summary.json shows no files below threshold
Files:
src/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.spec.tsxsrc/screens/OrganizationActionItems/ActionItemModal/ActionItemModal.spec.tsx
🧠 Learnings (10)
📚 Learning: 2025-12-26T20:34:53.912Z
Learnt from: MuhammadUmar7195
Repo: PalisadoesFoundation/talawa-admin PR: 5847
File: src/screens/UserPortal/Volunteer/Invitations/Invitations.tsx:243-243
Timestamp: 2025-12-26T20:34:53.912Z
Learning: In PalisadoesFoundation/talawa-admin, whenever you modify a file (e.g., for migrations, tests, or feature changes), replace inline color attributes like color="grey" with Bootstrap utility classes (e.g., className="text-secondary") to satisfy the CSS-Policy-Check CI. This is a general compliance guideline, not scope creep, and should be applied to all affected TSX files during reviews. Steps: 1) Find inline color attributes in JSX/TSX. 2) Replace with appropriate Bootstrap text/color utility classes. 3) Ensure the visual outcome remains the same. 4) Run CI checks to confirm the policy passes.
Applied to files:
src/components/AdminPortal/ApplyToSelector/ApplyToSelector.tsxsrc/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.spec.tsxsrc/screens/OrganizationActionItems/ActionItemModal/ActionItemModal.spec.tsxsrc/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.tsxsrc/screens/OrganizationActionItems/ActionItemModal/ActionItemModal.tsx
📚 Learning: 2026-01-06T16:39:41.033Z
Learnt from: Jashan32
Repo: PalisadoesFoundation/talawa-admin PR: 6199
File: src/shared-components/ProfileForm/ProfileForm.tsx:0-0
Timestamp: 2026-01-06T16:39:41.033Z
Learning: When refactoring or consolidating components (e.g., talawa-admin), remove helper functions that were copied from old files but are not used in the new implementation. Before exporting utilities, verify there are real usages across the codebase (search usages, remove dead code, and keep only necessary exports) to improve maintainability and reduce bundle size.
Applied to files:
src/components/AdminPortal/ApplyToSelector/ApplyToSelector.tsxsrc/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.spec.tsxsrc/screens/OrganizationActionItems/ActionItemModal/ActionItemModal.spec.tsxsrc/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.tsxsrc/screens/OrganizationActionItems/ActionItemModal/ActionItemModal.tsx
📚 Learning: 2026-01-07T09:08:09.150Z
Learnt from: adityai0
Repo: PalisadoesFoundation/talawa-admin PR: 6202
File: src/components/EventManagement/EventAgendaItems/EventAgendaItems.tsx:4-13
Timestamp: 2026-01-07T09:08:09.150Z
Learning: In the talawa-admin repository, document comments with TSDoc in all TSX files. Do not use JSDoc. Write non-standard tags like component and requires as plain text (no leading @). Use standard TSDoc tags such as param, remarks, returns, and example with the leading @. This guideline applies to all TSX components (e.g., src/components/.../*.tsx).
Applied to files:
src/components/AdminPortal/ApplyToSelector/ApplyToSelector.tsxsrc/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.spec.tsxsrc/screens/OrganizationActionItems/ActionItemModal/ActionItemModal.spec.tsxsrc/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.tsxsrc/screens/OrganizationActionItems/ActionItemModal/ActionItemModal.tsx
📚 Learning: 2026-01-08T16:45:49.201Z
Learnt from: adityai0
Repo: PalisadoesFoundation/talawa-admin PR: 6241
File: src/components/AdminPortal/OrgPeopleListCard/OrgPeopleListCard.tsx:12-16
Timestamp: 2026-01-08T16:45:49.201Z
Learning: In TSX components, prefer flat JSDoc parameter documentation format (e.g., param id, param toggleRemoveModal) over the nested format (param props.id). The nested form can break editor parsing. When a component accepts a props object, destructure the props in the function signature to align with the flat JSDoc style (e.g., function MyComp({ id, toggleRemoveModal }: Props) { ... }). Ensure all param tags correspond to the destructured props and reflect actual component props to improve editor support and readability.
Applied to files:
src/components/AdminPortal/ApplyToSelector/ApplyToSelector.tsxsrc/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.spec.tsxsrc/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.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/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.spec.tsxsrc/screens/OrganizationActionItems/ActionItemModal/ActionItemModal.spec.tsx
📚 Learning: 2025-12-27T11:22:41.191Z
Learnt from: rawadhossain
Repo: PalisadoesFoundation/talawa-admin PR: 5782
File: src/screens/Leaderboard/Leaderboard.spec.tsx:342-370
Timestamp: 2025-12-27T11:22:41.191Z
Learning: In test files that render screens with shared components (e.g., src/screens/Leaderboard/Leaderboard.spec.tsx), verify integration behavior by asserting the screen renders correctly in different scenarios (e.g., search results) without asserting the internal ARIA attributes, text, or icons of the shared component. Rely on the shared component's own tests to cover accessibility and presentation details. This keeps tests focused on higher-level behavior and interactions.
Applied to files:
src/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.spec.tsxsrc/screens/OrganizationActionItems/ActionItemModal/ActionItemModal.spec.tsx
📚 Learning: 2026-01-08T18:48:10.364Z
Learnt from: abhishekblue
Repo: PalisadoesFoundation/talawa-admin PR: 6260
File: src/screens/AdminPortal/OrganizationPeople/OrganizationPeople.spec.tsx:337-339
Timestamp: 2026-01-08T18:48:10.364Z
Learning: In tests that exercise Apollo Client queries (e.g., talawa-admin tests under src/screens/.../OrganizationPeople.spec.tsx), avoid using vi.useFakeTimers() because Apollo's internal promise resolution can be disrupted by mocked timers, causing timeouts. Use real timers with explicit waits (for example, await act(async () => { await new Promise(r => setTimeout(r, DURATION)); })) when testing components that fetch data via Apollo. If you must use timers, prefer real timers for the relevant test and flush promises deterministically. Replace or remove useFakeTimers in such tests and rely on actual time passage or proper promise flushing to ensure reliable Apollo-driven data fetching tests.
Applied to files:
src/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.spec.tsxsrc/screens/OrganizationActionItems/ActionItemModal/ActionItemModal.spec.tsx
📚 Learning: 2025-12-27T11:22:19.556Z
Learnt from: rawadhossain
Repo: PalisadoesFoundation/talawa-admin PR: 5782
File: src/screens/Leaderboard/Leaderboard.spec.tsx:299-329
Timestamp: 2025-12-27T11:22:19.556Z
Learning: In talawa-admin test files (e.g., src/screens/Leaderboard/Leaderboard.spec.tsx), prefer separate explicit test cases for keyboard navigation interactions (Enter key, Space key) instead of consolidating them with test.each()/parameterization. This improves readability, documents each keyboard interaction path clearly, and makes failures easier to diagnose when specific key handling regresses. Apply this pattern to similarly structured spec files by keeping individual tests for each key interaction rather than combining them into a single parameterized test.
Applied to files:
src/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.spec.tsxsrc/screens/OrganizationActionItems/ActionItemModal/ActionItemModal.spec.tsx
📚 Learning: 2026-01-06T21:27:50.880Z
Learnt from: Jashan32
Repo: PalisadoesFoundation/talawa-admin PR: 6222
File: src/utils/userUpdateUtils.ts:22-22
Timestamp: 2026-01-06T21:27:50.880Z
Learning: In the Talawa Admin codebase, the i18n extraction tool can misclassify TypeScript generic syntax (e.g., (obj: T): Partial) as translatable content. Suppress these false positives by adding a // i18n-ignore-next-line comment directly above the line with the type declaration or generic function signature. Apply this only to TS code where misclassification is confirmed; avoid blanket suppression.
Applied to files:
src/types/AdminPortal/AssignmentTypeSelector/interface.ts
📚 Learning: 2026-01-07T14:21:19.711Z
Learnt from: cotishq
Repo: PalisadoesFoundation/talawa-admin PR: 6198
File: src/setup/setup.ts:123-124
Timestamp: 2026-01-07T14:21:19.711Z
Learning: Use TSDoc (eslint-plugin-tsdoc) instead of JSDoc. Do not wrap type names in curly braces in throws and returns. Write as "throws Error" and "returns Promise<void>" (no braces). Curly-brace usage causes tsdoc-malformed-inline-tag and tsdoc-escape-right-brace lint errors. Apply this rule to all TypeScript source files (e.g., src/**/*.ts) to ensure consistency.
Applied to files:
src/types/AdminPortal/AssignmentTypeSelector/interface.ts
🧬 Code graph analysis (3)
src/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.spec.tsx (2)
src/types/AdminPortal/AssignmentTypeSelector/interface.ts (1)
AssignmentType(6-6)src/test-utils/I18nextProviderMock.tsx (1)
I18nextProvider(14-29)
src/types/AdminPortal/AssignmentTypeSelector/interface.ts (1)
src/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.tsx (1)
AssignmentType(9-9)
src/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.tsx (1)
src/types/AdminPortal/AssignmentTypeSelector/interface.ts (1)
InterfaceAssignmentTypeSelectorProps(11-24)
🪛 Biome (2.1.2)
src/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.tsx
[error] 52-52: The elements with this role can be changed to the following elements:
For examples and more information, see WAI-ARIA Roles
(lint/a11y/useSemanticElements)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Merge Coverage Reports
🔇 Additional comments (15)
src/screens/OrganizationActionItems/ActionItemModal/ActionItemModal.spec.tsx (3)
17-25: LGTM! Proper test lifecycle management.The expanded imports and top-level
afterEachwithvi.clearAllMocks()ensure proper test isolation per the repository guidelines. TheafterAllhook provides comprehensive cleanup at suite end.Also applies to: 61-63
970-970: Correct removal of async from describe blocks.The
describecallback should be synchronous; async operations belong init/testblocks. This change aligns with Vitest best practices.Also applies to: 1228-1228
1635-1638: Good suite-level cleanup.The
afterAllhook withvi.clearAllMocks()andvi.clearAllTimers()ensures complete cleanup after all tests complete.src/components/AdminPortal/ApplyToSelector/ApplyToSelector.tsx (2)
1-1: LGTM! Proper use of React 19's useId for accessible unique IDs.Using
useId()ensures stable, unique IDs that work correctly with SSR and prevent conflicts when multipleApplyToSelectorinstances are rendered. The// i18n-ignore-linecomments appropriately suppress false positives from the i18n extraction tool.Also applies to: 22-25
36-37: Dynamic IDs correctly applied to radio inputs.The radio inputs now use unique
nameandidattributes derived fromuseId(), ensuring proper form behavior and accessibility when multiple selectors are present.Also applies to: 44-45
src/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.spec.tsx (3)
10-35: Well-structured test suite with proper setup.The
renderComponenthelper provides flexible prop configuration, and theafterEachblock withvi.clearAllMocks()ensures test isolation per repository guidelines. The I18nextProvider setup correctly enables translation testing.
72-118: Good coverage of click handler behavior.Tests properly verify that:
onTypeChangeis called with the correct type value- The appropriate clear handler (
onClearVolunteer/onClearVolunteerGroup) is invoked when switching typesThis matches the component's expected behavior of clearing the opposite selection when switching assignment types.
120-166: Disabled state tests verify click prevention.Tests confirm that clicking disabled chips does not trigger
onTypeChangeor clear handlers, which is essential for the edit-mode behavior where assignment type cannot be switched.src/types/AdminPortal/AssignmentTypeSelector/interface.ts (1)
1-24: LGTM! Well-structured type definitions following repository conventions.The interface follows the required naming pattern (
InterfaceAssignmentTypeSelectorProps), is properly located insrc/types/AdminPortal/, and includes appropriate TSDoc comments. TheAssignmentTypeunion type provides strong typing for the assignment mode.src/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.tsx (3)
1-9: Good component setup with proper imports and type re-export.The component correctly imports from the interface file and re-exports
AssignmentTypefor consumer convenience, allowing imports like:import AssignmentTypeSelector, { AssignmentType } from 'components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector';
28-29: Good use of useId for accessible unique IDs.The
labelIdconnects the Typography label to the group viaaria-labelledby, ensuring screen readers announce the group purpose. ThegroupIdprovides a unique identifier for the group container.
56-79: Well-implemented accessible chip controls.The chips correctly:
- Use
aria-disabledfor accessibility state- Apply visual feedback via
opacityandcursorfor disabled state- Toggle between
filled/outlinedvariants for selection state- Use
primary/defaultcolor for visual distinctionsrc/screens/OrganizationActionItems/ActionItemModal/ActionItemModal.tsx (3)
1-6: Good module-level documentation.The file-level comment clearly describes the component's purpose and responsibilities. This helps developers understand the modal's role in the action items workflow.
130-134: Clear computed flag for gating applyTo relevance.The
isApplyToRelevantflag correctly encapsulates the complex conditional logic for when series/instance filtering should apply. This improves readability in the useMemo hooks and useEffect that depend on it.
194-202: Good use of useCallback for stable handler reference.Wrapping
handleFormChangeinuseCallbackwith an empty dependency array is correct since it only uses the state setter function (which is stable). This prevents unnecessary re-renders of child components that receive this callback.
| it('renders with disabled state styling when volunteer chip is disabled', () => { | ||
| renderComponent('volunteerGroup', vi.fn(), true, false); | ||
|
|
||
| const volunteerChip = screen | ||
| .getByText('Volunteer') | ||
| .closest('div[aria-disabled="true"]'); | ||
| expect(volunteerChip).toHaveAttribute('aria-disabled', 'true'); | ||
| }); | ||
|
|
||
| it('renders with disabled state styling when volunteer group chip is disabled', () => { | ||
| renderComponent('volunteer', vi.fn(), false, true); | ||
|
|
||
| const volunteerGroupChip = screen | ||
| .getByText('Volunteer Group') | ||
| .closest('div[aria-disabled="true"]'); | ||
| expect(volunteerGroupChip).toHaveAttribute('aria-disabled', 'true'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding keyboard accessibility tests.
While click interactions are well-tested, consider adding tests for keyboard navigation (Enter/Space key) on the chips to ensure accessibility compliance. Per repository learnings, prefer separate explicit test cases for each keyboard interaction.
♻️ Example keyboard test structure
it('handles Enter key on volunteer chip when not disabled', async () => {
const onTypeChange = vi.fn();
renderComponent('volunteerGroup', onTypeChange, false, false);
const user = userEvent.setup();
const volunteerChip = screen.getByText('Volunteer').closest('[role="button"]');
if (volunteerChip) {
volunteerChip.focus();
await user.keyboard('{Enter}');
}
expect(onTypeChange).toHaveBeenCalledWith('volunteer');
});
it('handles Space key on volunteer chip when not disabled', async () => {
// Similar structure for Space key
});| /** | ||
| * Chip-based toggle selector for choosing assignment type (volunteer or volunteer group). | ||
| */ |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
TSDoc comment should document props.
Per repository guidelines, TSDoc for exported components should document parameters. Consider expanding the documentation.
📝 Enhanced TSDoc
/**
* Chip-based toggle selector for choosing assignment type (volunteer or volunteer group).
+ *
+ * @param props - Component props from InterfaceAssignmentTypeSelectorProps
+ * @returns JSX.Element
*/📝 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.
| /** | |
| * Chip-based toggle selector for choosing assignment type (volunteer or volunteer group). | |
| */ | |
| /** | |
| * Chip-based toggle selector for choosing assignment type (volunteer or volunteer group). | |
| * | |
| * @param props - Component props from InterfaceAssignmentTypeSelectorProps | |
| * @returns JSX.Element | |
| */ |
🤖 Prompt for AI Agents
In @src/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.tsx
around lines 11 - 13, Update the TSDoc for the exported AssignmentTypeSelector
component to include a @param for the props object and document each prop the
component accepts (e.g., selected value, onChange handler, optional
className/aria props or any others used inside AssignmentTypeSelector), include
the expected types/allowed values and behavior for each prop, note any
defaults/optional props, and add a @returns line describing the React element;
place this expanded docblock directly above the AssignmentTypeSelector export.
| <Box | ||
| className="d-flex gap-2" | ||
| role="group" | ||
| aria-labelledby={labelId} | ||
| id={groupId} | ||
| > |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using semantic <fieldset> element for better accessibility.
The static analysis tool correctly identifies that role="group" can be replaced with a semantic <fieldset> element. This provides better native accessibility support without needing ARIA attributes.
♻️ Proposed semantic HTML improvement
- <Box
- className="d-flex gap-2"
- role="group"
- aria-labelledby={labelId}
- id={groupId}
- >
+ <Box
+ component="fieldset"
+ className="d-flex gap-2 border-0 p-0 m-0"
+ aria-labelledby={labelId}
+ id={groupId}
+ >Using component="fieldset" on MUI's Box renders a semantic <fieldset> element while preserving layout styles. Add border-0 p-0 m-0 to reset default fieldset styling.
📝 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.
| <Box | |
| className="d-flex gap-2" | |
| role="group" | |
| aria-labelledby={labelId} | |
| id={groupId} | |
| > | |
| <Box | |
| component="fieldset" | |
| className="d-flex gap-2 border-0 p-0 m-0" | |
| aria-labelledby={labelId} | |
| id={groupId} | |
| > |
🧰 Tools
🪛 Biome (2.1.2)
[error] 52-52: The elements with this role can be changed to the following elements:
For examples and more information, see WAI-ARIA Roles
(lint/a11y/useSemanticElements)
🤖 Prompt for AI Agents
In @src/components/AdminPortal/AssignmentTypeSelector/AssignmentTypeSelector.tsx
around lines 50 - 55, Replace the non-semantic Box group with a semantic
fieldset by rendering Box with component="fieldset" in AssignmentTypeSelector;
remove role="group" (native fieldset provides grouping) but keep
aria-labelledby={labelId} and id={groupId}, and add layout-reset classes like
"border-0 p-0 m-0 d-flex gap-2" so the visual styles are preserved while using
the native <fieldset>.
| if (isRecurring && applyTo === 'series' && !baseEvent?.id) { | ||
| NotificationToast.error({ | ||
| key: 'unknownError', | ||
| namespace: 'errors', | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| if (!isRecurring && !eventId) { | ||
| NotificationToast.error({ | ||
| key: 'unknownError', | ||
| namespace: 'errors', | ||
| }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using specific error keys instead of generic 'unknownError'.
These validation checks catch specific invalid states but display the same generic error for all cases. Users and developers won't be able to distinguish which condition failed.
♻️ Suggested improvement
Add specific translation keys for these scenarios:
if (isRecurring && applyTo === 'series' && !baseEvent?.id) {
NotificationToast.error({
- key: 'unknownError',
- namespace: 'errors',
+ key: 'missingBaseEventForSeries',
+ namespace: 'translation',
});
return;
}
if (!isRecurring && !eventId) {
NotificationToast.error({
- key: 'unknownError',
- namespace: 'errors',
+ key: 'missingEventId',
+ namespace: 'translation',
});
return;
}Then add the corresponding keys to all locale files under the organizationActionItems namespace.
🤖 Prompt for AI Agents
In @src/screens/OrganizationActionItems/ActionItemModal/ActionItemModal.tsx
around lines 215 - 229, Replace the generic unknownError toast with specific
translation keys for each validation branch so callers and users can distinguish
failures: when isRecurring && applyTo === 'series' && !baseEvent?.id, call
NotificationToast.error with a key like 'recurringSeriesMissingBaseEvent'
(namespace 'organizationActionItems') and when !isRecurring && !eventId, call
NotificationToast.error with a key like 'singleEventMissingId' (same namespace);
then add those keys and messages to all locale files under the
organizationActionItems namespace so translations exist for both cases.
| if (!eventId) { | ||
| NotificationToast.error({ | ||
| key: 'unknownError', | ||
| namespace: 'errors', | ||
| }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Same recommendation: use a specific error key for the missing eventId validation.
🤖 Prompt for AI Agents
In @src/screens/OrganizationActionItems/ActionItemModal/ActionItemModal.tsx
around lines 323 - 329, The validation currently logs a generic 'unknownError'
when eventId is missing in ActionItemModal (the if (!eventId) block); change the
NotificationToast.error call to use a specific i18n key (e.g. 'missingEventId'
or 'validation.eventIdRequired') and appropriate namespace so the toast maps to
a clear, localized validation message for the missing eventId; update any tests
or translation files that expect the old key accordingly.
| <AssignmentTypeSelector | ||
| assignmentType={assignmentType} | ||
| onTypeChange={setAssignmentType} | ||
| isVolunteerDisabled={isVolunteerChipDisabled} | ||
| isVolunteerGroupDisabled={isVolunteerGroupChipDisabled} | ||
| onClearVolunteer={() => { | ||
| handleFormChange('volunteerId', ''); | ||
| setSelectedVolunteer(null); | ||
| }} | ||
| onClearVolunteerGroup={() => { | ||
| handleFormChange('volunteerGroupId', ''); | ||
| setSelectedVolunteerGroup(null); | ||
| }} | ||
| /> |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LGTM! Consider memoizing callbacks for consistency.
The integration with AssignmentTypeSelector is correct and the props are properly wired. The inline arrow functions for onClearVolunteer and onClearVolunteerGroup work fine, but for consistency with the memoized handleFormChange, you could optionally extract these as useCallback hooks.
♻️ Optional: Memoize clear callbacks
const handleClearVolunteer = useCallback(() => {
handleFormChange('volunteerId', '');
setSelectedVolunteer(null);
}, [handleFormChange]);
const handleClearVolunteerGroup = useCallback(() => {
handleFormChange('volunteerGroupId', '');
setSelectedVolunteerGroup(null);
}, [handleFormChange]);Then pass these as onClearVolunteer={handleClearVolunteer} and onClearVolunteerGroup={handleClearVolunteerGroup}.
🤖 Prompt for AI Agents
In @src/screens/OrganizationActionItems/ActionItemModal/ActionItemModal.tsx
around lines 506 - 519, Extract the inline clear handlers passed to
AssignmentTypeSelector into memoized callbacks: create handleClearVolunteer and
handleClearVolunteerGroup using useCallback, each calling
handleFormChange('volunteerId','') / handleFormChange('volunteerGroupId','') and
setSelectedVolunteer(null) / setSelectedVolunteerGroup(null); include
handleFormChange in the dependency arrays (and any setters if lint requires) and
pass the memoized functions to AssignmentTypeSelector as onClearVolunteer and
onClearVolunteerGroup.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #6383 +/- ##
===========================================
- Coverage 96.73% 96.70% -0.04%
===========================================
Files 456 457 +1
Lines 16594 16621 +27
Branches 5184 5113 -71
===========================================
+ Hits 16052 16073 +21
- Misses 529 535 +6
Partials 13 13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I have taken the suggestions into the considerations and will commit the required changes |
There was a problem hiding this comment.
Please:
- Fix any failing tests
- Ensure test code coverage for your patch meets our minimum >=95% requirements
- Fix any CI/CD failures. Click on the
Detailslink for the tests to get an explanation of the cause(s)- The sensitive files check does not prevent the entire test suite from running.
- Ensure code rabbit approves your changes
- Remedy any conflicting files
- Ask coderabbit to do a “full review” prior to requesting a review from our volunteers.
2 participants
What kind of change does this PR introduce?
Refactoring : src/screens/OrganizationActionItems/ActionItemModal/ActionItemModal.tsx
Issue Number:
Fixes #6250
Snapshots/Videos:
N/A
If relevant, did you update the documentation?
N/A
Summary
I was assigned to the issue #6250 in which I had to refactor the src/screens/OrganizationActionItems/ActionItemModal/ActionItemModal.tsx File. This is Part 2/5 of the refactoring process. I saw the following issues that could be refactored :
Line count reduction: From 713 lines to 646 lines
Does this PR introduce a breaking change?
No
Checklist
CodeRabbit AI Review
Test Coverage
Other information
This is in Continuation with the Previous PR #6314
Have you read the contributing guide?
Yes
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.