Conversation
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughReplaces select-based role assignment with a modal-driven UI, adds a searchable RoleUserSelectionModal, consolidates role/user fetching into a new fetchAllForUnit API, and implements pending-assignment logic to prevent users from being assigned to multiple roles. Tests updated accordingly across role-related suites and sidebar/accounting. Changes
Sequence DiagramsequenceDiagram
participant User
participant UI as RoleAssignmentItem
participant Modal as RoleUserSelectionModal
participant Store as RolesStore
participant Server
User->>UI: Press row to assign user
UI->>Modal: open(isOpen=true, props: roleId, users, currentAssignments)
Modal->>Store: read users / currentAssignments
User->>Modal: select user (or Unassigned)
Modal->>UI: onSelectUser(userId or undefined)
UI->>Store: handleAssignUser(roleId, userId)
Store->>Store: update pendingAssignments (auto-unassign conflicting roles)
Store->>Store: compute effectiveAssignments (server + pending)
UI->>UI: re-render with effectiveAssignments
User->>UI: Press Save
UI->>Store: save pendingAssignments
Store->>Server: persist assignments
Server-->>Store: confirm
Store->>Store: clear pending state / refresh
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/components/roles/roles-bottom-sheet.tsx (1)
40-46:⚠️ Potential issue | 🟡 MinorNo guard against concurrent
fetchAllForUnitinvocations on rapid open/closeEvery
isOpen → truetransition firesfetchAllForUnit, which launches three concurrent API calls. If the sheet is closed and reopened before the first batch resolves, a second batch starts in parallel. Because there is no abort/cancellation, whichever batch resolves last wins, and that could be the older (stale) one.A lightweight guard—similar to what
initalready does—prevents the duplicate in-flight fetches:🛡️ Suggested guard
React.useEffect(() => { if (isOpen && activeUnit) { + if (useRolesStore.getState().isLoading) return; useRolesStore.getState().fetchAllForUnit(activeUnit.UnitId); setPendingAssignments([]); } }, [isOpen, activeUnit]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/roles/roles-bottom-sheet.tsx` around lines 40 - 46, The effect currently calls useRolesStore.getState().fetchAllForUnit on every isOpen true transition which can start overlapping fetch batches; add a lightweight guard in the effect to avoid starting a second fetch when one is already in-flight for the same unit (e.g. query a store flag like useRolesStore.getState().isFetchingForUnit(activeUnit.UnitId) or compare useRolesStore.getState().currentFetchToken/unitId before calling fetchAllForUnit), or update fetchAllForUnit to accept/return a cancel token and store the active token so the effect can skip invocation if a token exists; modify the effect around React.useEffect(...) so it checks that guard (and still calls setPendingAssignments([]) when opening).src/components/roles/roles-modal.tsx (2)
162-162:⚠️ Potential issue | 🟡 MinorError message is not wrapped in
t()for internationalization.The
errorstring from the store is displayed directly. All user-facing text should be wrapped int(). As per coding guidelines, "Ensure all text is wrapped int()fromreact-i18nextfor translations."♻️ Suggested change
- <Text className="py-4 text-center text-red-500">{error}</Text> + <Text className="py-4 text-center text-red-500">{t('common.error', error ?? 'An error occurred')}</Text>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/roles/roles-modal.tsx` at line 162, The displayed error string must be internationalized: import and call useTranslation (const { t } = useTranslation()) inside the RolesModal component and replace the raw {error} in the Text element with a translated value (e.g., {t(error)} if error is a translation key, or {t('roles.error', { message: error })} if error is a raw message and you add a template key), updating the Text node referenced in roles-modal.tsx accordingly.
144-144:⚠️ Potential issue | 🟡 MinorToast error message is not internationalized.
Hardcoded English string passed to
showToast. As per coding guidelines, "Ensure all text is wrapped int()fromreact-i18nextfor translations."♻️ Suggested change
- useToastStore.getState().showToast('error', 'Error saving role assignments'); + useToastStore.getState().showToast('error', t('roles.errors.saveFailed', 'Error saving role assignments'));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/roles/roles-modal.tsx` at line 144, The toast error message in roles-modal.tsx currently passes a hardcoded English string to useToastStore.getState().showToast; replace that literal with a translated string using react-i18next (e.g., call t('roles.saveError') or similar) and ensure the translation function is available in the component by importing useTranslation and doing const { t } = useTranslation(); then pass t('...') to useToastStore.getState().showToast('error', t('roles.saveError')) so the message is internationalized.
🧹 Nitpick comments (12)
src/components/roles/role-user-selection-modal.tsx (1)
64-70: Redundantkeyprop onPressableinsiderenderUserItem
renderUserItemis called as a plain function insidefilteredUsers.map(...), where the wrappingReact.Fragmentalready carrieskey={user.UserId}(line 178). Thekeyon the innerPressable(line 66) is unused by the reconciler.♻️ Suggested cleanup
- <Pressable - key={item.UserId} - onPress={() => handleSelect(item.UserId)} + <Pressable + onPress={() => handleSelect(item.UserId)}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/roles/role-user-selection-modal.tsx` around lines 64 - 70, The inner Pressable in renderUserItem is given a redundant key prop (key={item.UserId}) even though the outer React.Fragment rendered in filteredUsers.map already provides key={user.UserId}; remove the unused key on the Pressable inside renderUserItem (the element created in the renderUserItem function) so only the fragment's key remains, leaving onPress, className, testID and accessibilityRole unchanged.src/components/roles/__tests__/roles-modal.test.tsx (1)
289-365: "Empty RoleId prevention" tests verify inline logic, not the component'shandleSaveTests like "should allow unassignments by including roles with valid RoleId but empty UserId" (lines 316–332) and "should find role assignments without UnitId filter" (lines 343–364) exercise copy-pasted filter/find lambdas directly. If the component's
handleSavediverges from these inline snippets, the tests continue to pass while the real behavior is broken.Consider either:
- Replacing inline-logic tests with integration-style tests that press the Save button on a rendered
<RolesModal>and assertmockAssignRoleswas called with the expected payload, or- Moving the shared filter logic to a pure utility function and unit-testing that utility directly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/roles/__tests__/roles-modal.test.tsx` around lines 289 - 365, The tests under "Empty RoleId prevention" exercise inline filter/find lambdas rather than the component's actual save logic, so they can pass even if RolesModal.handleSave is broken; change the tests to either (a) be integration tests that render <RolesModal isOpen onClose={mockOnClose} />, simulate user actions and press the Save button, then assert mockAssignRoles was called with the expected payload (verifying unassignments where RoleId is present but UserId is empty and that UnitId is not required), or (b) extract the filtering/find logic into a pure utility (e.g., a new function like filterValidRoleAssignments or findAssignmentByUnitRoleId) and unit-test that utility directly instead of re-implementing lambdas in the test file so behavior and implementation stay in sync with RolesModal.handleSave.src/components/roles/__tests__/role-user-selection-modal.test.tsx (2)
292-307: Empty-state test doesn't verify the fallback text is renderedThe test confirms user items disappear but doesn't assert that the "No users found" message appears. A search that matches nothing could silently render nothing if the empty state branch is broken.
🧪 Suggested addition
fireEvent.changeText(screen.getByTestId('user-search-input'), 'zzzzz'); expect(screen.queryByTestId('user-item-user1')).toBeNull(); expect(screen.queryByTestId('user-item-user2')).toBeNull(); + expect(screen.getByText('No users found')).toBeTruthy();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/roles/__tests__/role-user-selection-modal.test.tsx` around lines 292 - 307, The test "shows empty state when no users match search" for RoleUserSelectionModal currently only checks that user items ('user-item-user1', 'user-item-user2') are gone; update the test to also assert the empty-state fallback is rendered by checking for the "No users found" message (e.g. via screen.getByText('No users found') or the modal's empty-state test id) after firing changeText on 'user-search-input' so the empty-state branch is explicitly verified.
96-325: Missing coverage forcurrentAssignments/ "assigned elsewhere" badgeThe new
currentAssignments+currentRoleIdprops drive the amber "assigned elsewhere" badge in the component, but no test exercises that path. Given the latent bug described in the component review below, a dedicated test here would both document the expected behavior and catch regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/roles/__tests__/role-user-selection-modal.test.tsx` around lines 96 - 325, Add a test for RoleUserSelectionModal that covers the currentAssignments/currentRoleId path: render RoleUserSelectionModal with isOpen true, users mockUsers, currentRoleId set to a different role than the assignment, and currentAssignments containing an entry mapping one of the UserId values (e.g., 'user2') to an object indicating they are assigned to another role; then assert that the "assigned elsewhere" (amber) badge is present for that user's test id (e.g., 'user-item-user2' or a badge test id), and verify selection behavior still works (tapping the user still calls onSelectUser/onClose or is prevented as expected by the component). Ensure you reference the component props currentAssignments and currentRoleId and the RoleUserSelectionModal render used in existing tests.src/stores/roles/store.ts (1)
101-119:fetchAllForUnitlacks a concurrency guard unlikeinit
initguards against concurrent runs via theisLoading/isInitializedcheck, butfetchAllForUnitstarts unconditionally. If called while still in-flight (e.g., the sheet opened twice in quick succession), two concurrentPromise.allbatches race to overwrite the store, and the earlier response may land last.♻️ Suggested guard
fetchAllForUnit: async (unitId: string) => { + if (useRolesStore.getState().isLoading) return; set({ isLoading: true, error: null });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stores/roles/store.ts` around lines 101 - 119, fetchAllForUnit currently always starts a fetch and can run concurrently with another call; guard it like init by reading the store state (via get()) and returning early if a fetch is already in progress (e.g., if get().isLoading is true), then proceed to set isLoading and run the Promise.all of getAllUnitRolesAndAssignmentsForDepartment, getRoleAssignmentsForUnit, and getAllPersonnelInfos; also ensure you clear isLoading in a finally block so concurrent callers don't leave the store stuck. Use the existing symbols fetchAllForUnit, getRoleAssignmentsForUnit, getAllUnitRolesAndAssignmentsForDepartment, getAllPersonnelInfos, set and get to implement the guard and robust loading lifecycle.src/components/roles/__tests__/role-assignment-item.test.tsx (2)
27-31:mockModalOnSelectUseris assigned but never read in any test — dead code.The module-level variable is set on every render of the mock (line 31) and reset in
beforeEach(line 129), but no test ever accesses it. All interactions go throughfireEvent.pressinstead. Consider removing it to reduce noise.♻️ Suggested change
-let mockModalOnSelectUser: ((userId?: string) => void) | undefined; jest.mock('../role-user-selection-modal', () => ({ RoleUserSelectionModal: ({ isOpen, onClose, roleName, selectedUserId, users, onSelectUser }: any) => { const { View, Text, TouchableOpacity } = require('react-native'); - mockModalOnSelectUser = onSelectUser; if (!isOpen) return null;And in
beforeEach:beforeEach(() => { jest.clearAllMocks(); - mockModalOnSelectUser = undefined; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/roles/__tests__/role-assignment-item.test.tsx` around lines 27 - 31, The module-level variable mockModalOnSelectUser is assigned inside the RoleUserSelectionModal mock but never read in tests, so remove the dead variable and its resets in beforeEach; update the mock (RoleUserSelectionModal) to simply accept props and render without assigning onSelectUser to mockModalOnSelectUser, and delete any beforeEach lines that set mockModalOnSelectUser = undefined to keep tests clean.
29-29: Mock usesanyfor props typing.Per coding guidelines: "Avoid using
any; strive for precise types." The mock's props parameter usesany. While this is in a test mock and pragmatically acceptable, you could import or inline the relevant prop types for better type safety.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/roles/__tests__/role-assignment-item.test.tsx` at line 29, The test mock for RoleUserSelectionModal currently types its props as any; replace that with the real prop type to avoid any—import or inline the component's prop interface (e.g., RoleUserSelectionModalProps or the prop type used by RoleUserSelectionModal) and use it in the mock signature (props: RoleUserSelectionModalProps) so isOpen, onClose, roleName, selectedUserId, users, and onSelectUser are strongly typed.src/components/roles/role-assignment-item.tsx (1)
99-99: Consider breaking the longRoleUserSelectionModalprop list across multiple lines for readability.This single line is very wide with 8 props. Spreading them across lines would improve readability and make diffs cleaner.
♻️ Suggested formatting
- <RoleUserSelectionModal isOpen={isModalOpen} onClose={handleCloseModal} roleName={role.Name} selectedUserId={assignedUser?.UserId} users={availableUsers} onSelectUser={handleSelectUser} currentAssignments={currentAssignments} currentRoleId={role.UnitRoleId} /> + <RoleUserSelectionModal + isOpen={isModalOpen} + onClose={handleCloseModal} + roleName={role.Name} + selectedUserId={assignedUser?.UserId} + users={availableUsers} + onSelectUser={handleSelectUser} + currentAssignments={currentAssignments} + currentRoleId={role.UnitRoleId} + />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/roles/role-assignment-item.tsx` at line 99, The RoleUserSelectionModal JSX invocation is a single very long line; split the props onto multiple lines to improve readability: break the <RoleUserSelectionModal ... /> call so each prop (isOpen, onClose, roleName, selectedUserId, users, onSelectUser, currentAssignments, currentRoleId) appears on its own or grouped across lines, keeping the component name and closing tag aligned and preserving the existing prop order and values (e.g., isOpen={isModalOpen}, onClose={handleCloseModal}, selectedUserId={assignedUser?.UserId}, currentRoleId={role.UnitRoleId}).src/components/roles/roles-modal.tsx (4)
35-41:activeUnitis an object reference — consider depending on itsUnitIdinstead.If
useCoreStorereturns a new object with the sameUnitIdon re-render, this effect will re-fire and re-fetch unnecessarily. Depending onactiveUnit?.UnitIdand guarding inside the effect is more stable.♻️ Suggested change
React.useEffect(() => { if (isOpen && activeUnit) { useRolesStore.getState().fetchAllForUnit(activeUnit.UnitId); // Reset pending assignments when modal opens setPendingAssignments([]); } - }, [isOpen, activeUnit]); + }, [isOpen, activeUnit?.UnitId]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/roles/roles-modal.tsx` around lines 35 - 41, The effect currently depends on the object reference activeUnit which can trigger unnecessary re-runs; change the dependency to activeUnit?.UnitId and inside the effect guard on isOpen and activeUnit?.UnitId (or store UnitId in a local variable) so you call useRolesStore.getState().fetchAllForUnit(activeUnit.UnitId) and setPendingAssignments([]) only when isOpen is true and a UnitId exists; this ensures fetchAllForUnit and setPendingAssignments are triggered only when the unit identity changes rather than when a new object reference is returned.
106-146: Save doesn't resetpendingAssignmentson success before closing.On success (line 135-136), the modal closes but
pendingAssignmentsisn't cleared until the next open (line 39). If there's any re-render betweenonClose()and the nextisOpen=true, the stale pending state could briefly produce incorrecteffectiveAssignments. Consider resetting explicitly on successful save.♻️ Suggested change
// Refresh role assignments after all updates await useRolesStore.getState().fetchRolesForUnit(activeUnit.UnitId); + setPendingAssignments([]); onClose();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/roles/roles-modal.tsx` around lines 106 - 146, The save flow in handleSave successfully updates remote roles but never clears the local pendingAssignments before calling onClose, leaving stale pending state; after the await useRolesStore.getState().fetchRolesForUnit(activeUnit.UnitId) and before onClose(), explicitly reset the pendingAssignments state (e.g., call the setter that manages pendingAssignments or dispatch the clear action) so pendingAssignments is emptied on success; reference handleSave, pendingAssignments, useRolesStore.getState().fetchRolesForUnit, and onClose when making this change.
43-67: The auto-unassign swap logic is complex — consider adding inline comments for each branch.The logic is functionally correct: when assigning a user to a new role, it checks both server-side and pending assignments to automatically unassign the user from their previous role. However, the multiple nested conditionals and array manipulations make this hard to follow during future maintenance.
One concrete concern: the
pendingForUserbranch (lines 55-61) removes the pending entry for the user on the other role and, if there was a server assignment, replaces it with an explicit unassignment. But if the user was pending-assigned to two other roles (an unlikely edge case from rapid interactions),findon line 51 only catches the first one.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/roles/roles-modal.tsx` around lines 43 - 67, handleAssignUser's auto-unassign swap logic is hard to follow and may miss cases where a user has multiple pending assignments; add concise inline comments explaining each branch (the initial filter, serverAssignment detection via unitRoleAssignments, pendingForUser handling, and why we add explicit unassignment entries), and change the pendingForUser discovery from a single find to handling all matching pending assignments (e.g., iterate/filter pending entries for userId rather than using pendingForUser = updated.find(...)) so every pending assignment for that user is removed and, for each removed role that had a server assignment (check via unitRoleAssignments), add the explicit unassignment entry; reference the handleAssignUser callback, pendingAssignments state updater, unitRoleAssignments, pendingForUser logic, and the code paths that append { roleId, userId } or { roleId, userId: undefined } when applying these fixes.
166-181: Anonymous closure inmapcallback creates new function reference per role on every render.Line 179 creates a new arrow function
(userId) => handleAssignUser(role.UnitRoleId, userId)for each role on every render. While this is aScrollView(not aFlatList), it still causes everyRoleAssignmentItemto receive a newonAssignUserprop each render, defeatingReact.memoif it were applied. This is acceptable for a small list of roles but worth noting. As per coding guidelines, "Avoid anonymous functions in renderItem or event handlers to prevent re-renders."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/roles/roles-modal.tsx` around lines 166 - 181, The inline arrow passed to RoleAssignmentItem (onAssignUser={(userId) => handleAssignUser(role.UnitRoleId, userId)}) creates a new function per role every render; fix by building stable callbacks once and reusing them: create a memoized map of handlers (e.g., onAssignUserMap) with useMemo that iterates filteredRoles and stores a function for each role.UnitRoleId that calls handleAssignUser(roleId, userId), then pass onAssignUser={onAssignUserMap.get(role.UnitRoleId)} into RoleAssignmentItem so the onAssignUser reference is stable across renders unless filteredRoles or handleAssignUser change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/roles/role-user-selection-modal.tsx`:
- Around line 57-63: The logic in renderUserItem marks everyone "In another
role" when currentRoleId is undefined; update the
otherAssignment/isAssignedElsewhere check to only compare roleId when
currentRoleId is defined—i.e., when searching currentAssignments in
renderUserItem, include a guard that currentRoleId != null (or !== undefined)
before evaluating a.roleId !== currentRoleId so users assigned to the shown role
are not incorrectly flagged; reference renderUserItem, currentAssignments,
currentRoleId, otherAssignment, and isAssignedElsewhere when making the change.
---
Outside diff comments:
In `@src/components/roles/roles-bottom-sheet.tsx`:
- Around line 40-46: The effect currently calls
useRolesStore.getState().fetchAllForUnit on every isOpen true transition which
can start overlapping fetch batches; add a lightweight guard in the effect to
avoid starting a second fetch when one is already in-flight for the same unit
(e.g. query a store flag like
useRolesStore.getState().isFetchingForUnit(activeUnit.UnitId) or compare
useRolesStore.getState().currentFetchToken/unitId before calling
fetchAllForUnit), or update fetchAllForUnit to accept/return a cancel token and
store the active token so the effect can skip invocation if a token exists;
modify the effect around React.useEffect(...) so it checks that guard (and still
calls setPendingAssignments([]) when opening).
In `@src/components/roles/roles-modal.tsx`:
- Line 162: The displayed error string must be internationalized: import and
call useTranslation (const { t } = useTranslation()) inside the RolesModal
component and replace the raw {error} in the Text element with a translated
value (e.g., {t(error)} if error is a translation key, or {t('roles.error', {
message: error })} if error is a raw message and you add a template key),
updating the Text node referenced in roles-modal.tsx accordingly.
- Line 144: The toast error message in roles-modal.tsx currently passes a
hardcoded English string to useToastStore.getState().showToast; replace that
literal with a translated string using react-i18next (e.g., call
t('roles.saveError') or similar) and ensure the translation function is
available in the component by importing useTranslation and doing const { t } =
useTranslation(); then pass t('...') to
useToastStore.getState().showToast('error', t('roles.saveError')) so the message
is internationalized.
---
Nitpick comments:
In `@src/components/roles/__tests__/role-assignment-item.test.tsx`:
- Around line 27-31: The module-level variable mockModalOnSelectUser is assigned
inside the RoleUserSelectionModal mock but never read in tests, so remove the
dead variable and its resets in beforeEach; update the mock
(RoleUserSelectionModal) to simply accept props and render without assigning
onSelectUser to mockModalOnSelectUser, and delete any beforeEach lines that set
mockModalOnSelectUser = undefined to keep tests clean.
- Line 29: The test mock for RoleUserSelectionModal currently types its props as
any; replace that with the real prop type to avoid any—import or inline the
component's prop interface (e.g., RoleUserSelectionModalProps or the prop type
used by RoleUserSelectionModal) and use it in the mock signature (props:
RoleUserSelectionModalProps) so isOpen, onClose, roleName, selectedUserId,
users, and onSelectUser are strongly typed.
In `@src/components/roles/__tests__/role-user-selection-modal.test.tsx`:
- Around line 292-307: The test "shows empty state when no users match search"
for RoleUserSelectionModal currently only checks that user items
('user-item-user1', 'user-item-user2') are gone; update the test to also assert
the empty-state fallback is rendered by checking for the "No users found"
message (e.g. via screen.getByText('No users found') or the modal's empty-state
test id) after firing changeText on 'user-search-input' so the empty-state
branch is explicitly verified.
- Around line 96-325: Add a test for RoleUserSelectionModal that covers the
currentAssignments/currentRoleId path: render RoleUserSelectionModal with isOpen
true, users mockUsers, currentRoleId set to a different role than the
assignment, and currentAssignments containing an entry mapping one of the UserId
values (e.g., 'user2') to an object indicating they are assigned to another
role; then assert that the "assigned elsewhere" (amber) badge is present for
that user's test id (e.g., 'user-item-user2' or a badge test id), and verify
selection behavior still works (tapping the user still calls
onSelectUser/onClose or is prevented as expected by the component). Ensure you
reference the component props currentAssignments and currentRoleId and the
RoleUserSelectionModal render used in existing tests.
In `@src/components/roles/__tests__/roles-modal.test.tsx`:
- Around line 289-365: The tests under "Empty RoleId prevention" exercise inline
filter/find lambdas rather than the component's actual save logic, so they can
pass even if RolesModal.handleSave is broken; change the tests to either (a) be
integration tests that render <RolesModal isOpen onClose={mockOnClose} />,
simulate user actions and press the Save button, then assert mockAssignRoles was
called with the expected payload (verifying unassignments where RoleId is
present but UserId is empty and that UnitId is not required), or (b) extract the
filtering/find logic into a pure utility (e.g., a new function like
filterValidRoleAssignments or findAssignmentByUnitRoleId) and unit-test that
utility directly instead of re-implementing lambdas in the test file so behavior
and implementation stay in sync with RolesModal.handleSave.
In `@src/components/roles/role-assignment-item.tsx`:
- Line 99: The RoleUserSelectionModal JSX invocation is a single very long line;
split the props onto multiple lines to improve readability: break the
<RoleUserSelectionModal ... /> call so each prop (isOpen, onClose, roleName,
selectedUserId, users, onSelectUser, currentAssignments, currentRoleId) appears
on its own or grouped across lines, keeping the component name and closing tag
aligned and preserving the existing prop order and values (e.g.,
isOpen={isModalOpen}, onClose={handleCloseModal},
selectedUserId={assignedUser?.UserId}, currentRoleId={role.UnitRoleId}).
In `@src/components/roles/role-user-selection-modal.tsx`:
- Around line 64-70: The inner Pressable in renderUserItem is given a redundant
key prop (key={item.UserId}) even though the outer React.Fragment rendered in
filteredUsers.map already provides key={user.UserId}; remove the unused key on
the Pressable inside renderUserItem (the element created in the renderUserItem
function) so only the fragment's key remains, leaving onPress, className, testID
and accessibilityRole unchanged.
In `@src/components/roles/roles-modal.tsx`:
- Around line 35-41: The effect currently depends on the object reference
activeUnit which can trigger unnecessary re-runs; change the dependency to
activeUnit?.UnitId and inside the effect guard on isOpen and activeUnit?.UnitId
(or store UnitId in a local variable) so you call
useRolesStore.getState().fetchAllForUnit(activeUnit.UnitId) and
setPendingAssignments([]) only when isOpen is true and a UnitId exists; this
ensures fetchAllForUnit and setPendingAssignments are triggered only when the
unit identity changes rather than when a new object reference is returned.
- Around line 106-146: The save flow in handleSave successfully updates remote
roles but never clears the local pendingAssignments before calling onClose,
leaving stale pending state; after the await
useRolesStore.getState().fetchRolesForUnit(activeUnit.UnitId) and before
onClose(), explicitly reset the pendingAssignments state (e.g., call the setter
that manages pendingAssignments or dispatch the clear action) so
pendingAssignments is emptied on success; reference handleSave,
pendingAssignments, useRolesStore.getState().fetchRolesForUnit, and onClose when
making this change.
- Around line 43-67: handleAssignUser's auto-unassign swap logic is hard to
follow and may miss cases where a user has multiple pending assignments; add
concise inline comments explaining each branch (the initial filter,
serverAssignment detection via unitRoleAssignments, pendingForUser handling, and
why we add explicit unassignment entries), and change the pendingForUser
discovery from a single find to handling all matching pending assignments (e.g.,
iterate/filter pending entries for userId rather than using pendingForUser =
updated.find(...)) so every pending assignment for that user is removed and, for
each removed role that had a server assignment (check via unitRoleAssignments),
add the explicit unassignment entry; reference the handleAssignUser callback,
pendingAssignments state updater, unitRoleAssignments, pendingForUser logic, and
the code paths that append { roleId, userId } or { roleId, userId: undefined }
when applying these fixes.
- Around line 166-181: The inline arrow passed to RoleAssignmentItem
(onAssignUser={(userId) => handleAssignUser(role.UnitRoleId, userId)}) creates a
new function per role every render; fix by building stable callbacks once and
reusing them: create a memoized map of handlers (e.g., onAssignUserMap) with
useMemo that iterates filteredRoles and stores a function for each
role.UnitRoleId that calls handleAssignUser(roleId, userId), then pass
onAssignUser={onAssignUserMap.get(role.UnitRoleId)} into RoleAssignmentItem so
the onAssignUser reference is stable across renders unless filteredRoles or
handleAssignUser change.
In `@src/stores/roles/store.ts`:
- Around line 101-119: fetchAllForUnit currently always starts a fetch and can
run concurrently with another call; guard it like init by reading the store
state (via get()) and returning early if a fetch is already in progress (e.g.,
if get().isLoading is true), then proceed to set isLoading and run the
Promise.all of getAllUnitRolesAndAssignmentsForDepartment,
getRoleAssignmentsForUnit, and getAllPersonnelInfos; also ensure you clear
isLoading in a finally block so concurrent callers don't leave the store stuck.
Use the existing symbols fetchAllForUnit, getRoleAssignmentsForUnit,
getAllUnitRolesAndAssignmentsForDepartment, getAllPersonnelInfos, set and get to
implement the guard and robust loading lifecycle.
Member
Author
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary by CodeRabbit
New Features
Bug Fixes
Style
Tests
Chores