Open
Conversation
📝 WalkthroughWalkthroughMostly JSX/formatting refactors across client, center, and group pages; summary panels expanded to show separate group/client metrics; one functional change adds a password visibility toggle that preserves cursor position in the login form. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pages/groups/groups-view/group-actions/ManageGroupMembers.tsx (1)
117-126:⚠️ Potential issue | 🟠 MajorPersist the member removal before updating local state.
This flow only filters
clientMembersin memory. After a reload, the member will reappear because nothing is sent to the backend. Please call the group-member removal API here and update local state only after that request succeeds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/groups/groups-view/group-actions/ManageGroupMembers.tsx` around lines 117 - 126, In removeClient, instead of immediately filtering clientMembers in memory, call the backend group-member removal API (e.g., await removeGroupMember or deleteGroupMember with the group id and client.id) after user confirmation and before calling setClientMembers; only update state (setClientMembers(prev => prev.filter(...))) once the API returns success, handle failures by showing an error and not mutating state, and ensure setBusy(true) is cleared in a finally block; use the existing symbols removeClient, id, client.id, setClientMembers and setBusy to locate and implement the change.
🧹 Nitpick comments (1)
src/pages/login/Login.tsx (1)
66-81: Add a regression test for focus and caret restoration.This fix is browser-sensitive and easy to regress. A component test that types into the password field, toggles visibility, and asserts both focus retention and caret position would lock the bugfix in.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/login/Login.tsx` around lines 66 - 81, Add a regression component test for togglePasswordVisibility that ensures focus and caret are preserved: render the Login component, type into the password input referenced by passwordRef, move the caret to a non-edge position, trigger the visibility toggle (the handler that calls setShowPassword and then focuses and calls setSelectionRange inside setTimeout), and assert the input still has document.activeElement and that its selectionStart/selectionEnd match the prior caret positions; this test should fail if focus or selection restoration regresses.
🤖 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/pages/groups/groups-view/GroupsView.tsx`:
- Around line 234-236: The center label is rendering the group name; update the
JSX in GroupsView.tsx to display the center's name instead by replacing
group?.name with group?.center?.name and, if that is undefined, fall back to
t('view.missingInOpenAPI') (e.g. use group?.center?.name ??
t('view.missingInOpenAPI')). Locate this change where the div currently contains
{t('view.centerName')} {group?.name} and keep the surrounding label text intact.
In `@src/pages/login/Login.tsx`:
- Around line 66-81: The togglePasswordVisibility handler currently always
refocuses the password input, stealing focus from the toggling control; update
togglePasswordVisibility to first detect whether the input had focus (e.g.,
const hadFocus = document.activeElement === passwordRef.current) before flipping
show state, then only call passwordRef.current.focus() and
setSelectionRange(...) inside the setTimeout when hadFocus is true; keep using
passwordRef, selectionStart/selectionEnd and setShowPassword as currently
implemented but gate the restore logic on hadFocus.
---
Outside diff comments:
In `@src/pages/groups/groups-view/group-actions/ManageGroupMembers.tsx`:
- Around line 117-126: In removeClient, instead of immediately filtering
clientMembers in memory, call the backend group-member removal API (e.g., await
removeGroupMember or deleteGroupMember with the group id and client.id) after
user confirmation and before calling setClientMembers; only update state
(setClientMembers(prev => prev.filter(...))) once the API returns success,
handle failures by showing an error and not mutating state, and ensure
setBusy(true) is cleared in a finally block; use the existing symbols
removeClient, id, client.id, setClientMembers and setBusy to locate and
implement the change.
---
Nitpick comments:
In `@src/pages/login/Login.tsx`:
- Around line 66-81: Add a regression component test for
togglePasswordVisibility that ensures focus and caret are preserved: render the
Login component, type into the password input referenced by passwordRef, move
the caret to a non-edge position, trigger the visibility toggle (the handler
that calls setShowPassword and then focuses and calls setSelectionRange inside
setTimeout), and assert the input still has document.activeElement and that its
selectionStart/selectionEnd match the prior caret positions; this test should
fail if focus or selection restoration regresses.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3291637c-9dca-41e2-867e-d2277ab1b2cf
📒 Files selected for processing (16)
src/pages/centers/centers-view/general-tab/CentersGeneralTab.tsxsrc/pages/clients/Clients.tsxsrc/pages/clients/clients-view/ClientsView.tsxsrc/pages/clients/clients-view/Identities-tab/ClientsidentitiesTab.tsxsrc/pages/clients/clients-view/address-tab/ClientsAddressTab.tsxsrc/pages/clients/clients-view/charges/view-charge/ViewCharge.tsxsrc/pages/clients/clients-view/family-members-tab/ClientsFamilyMembersAddTab.tsxsrc/pages/clients/clients-view/family-members-tab/ClientsFamilyMembersTab.tsxsrc/pages/clients/clients-view/general-tab/ClientsGeneralTab.tsxsrc/pages/groups/groups-view/GroupsView.tsxsrc/pages/groups/groups-view/general-tab/GroupsGeneralTab.tsxsrc/pages/groups/groups-view/group-actions/AttachMeeting.tsxsrc/pages/groups/groups-view/group-actions/EditGroups.tsxsrc/pages/groups/groups-view/group-actions/ManageGroupMembers.tsxsrc/pages/groups/groups-view/group-actions/TransferClients.tsxsrc/pages/login/Login.tsx
Comment on lines
+234
to
+236
| <div> | ||
| {t('view.centerName')} {group?.name} | ||
| </div> |
There was a problem hiding this comment.
Render the actual center name here, not the group name.
Line 235 shows group?.name under the center label, so the UI repeats the group name and displays the wrong value for groups attached to a center. If this response does not include a center-name field, fall back to t('view.missingInOpenAPI') instead of duplicating the group name.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/groups/groups-view/GroupsView.tsx` around lines 234 - 236, The
center label is rendering the group name; update the JSX in GroupsView.tsx to
display the center's name instead by replacing group?.name with
group?.center?.name and, if that is undefined, fall back to
t('view.missingInOpenAPI') (e.g. use group?.center?.name ??
t('view.missingInOpenAPI')). Locate this change where the div currently contains
{t('view.centerName')} {group?.name} and keep the surrounding label text intact.
Mohit-Davar
force-pushed
the
MXWAR-72-fix-login-password-cursor
branch
from
5de72be to
2fb665b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/pages/groups/groups-view/GroupsView.tsx (1)
234-236:⚠️ Potential issue | 🟡 MinorRender the center's name here, not the group's name.
Line 235 still binds
{group?.name}under the center label, so the UI repeats the group name instead of showing the linked center. This should use the center name field and fall back tot('view.missingInOpenAPI')when it is absent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/groups/groups-view/GroupsView.tsx` around lines 234 - 236, Replace the mistaken binding that shows the group's name under the center label: in GroupsView (the JSX block rendering "{t('view.centerName')} {group?.name}") use the linked center's name (e.g., group?.center?.name or center?.name depending on how the center is exposed) and fall back to t('view.missingInOpenAPI') when absent; update the expression in that div to display the center name with a nullish/coalescing or conditional fallback to t('view.missingInOpenAPI').
🤖 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/pages/groups/groups-view/GroupsView.tsx`:
- Around line 96-99: The menu options are being created before group.id is
available causing transient paths like `groups/undefined/...`; update the logic
that builds `menuOptions` (the block that calls opts.push with label
t('view.menu.edit') and path `groups/${group?.id}/edit`) to only add
edit/view/delete entries when `group?.id` is truthy, or alternatively keep the
dropdown component disabled until the record loads (e.g., use the same group
loading boolean used by GroupsView to gate the menu rendering); ensure you apply
the same guard to the other pushes around lines where `groups/${group?.id}/...`
are created so no menu item is produced with an undefined id.
---
Duplicate comments:
In `@src/pages/groups/groups-view/GroupsView.tsx`:
- Around line 234-236: Replace the mistaken binding that shows the group's name
under the center label: in GroupsView (the JSX block rendering
"{t('view.centerName')} {group?.name}") use the linked center's name (e.g.,
group?.center?.name or center?.name depending on how the center is exposed) and
fall back to t('view.missingInOpenAPI') when absent; update the expression in
that div to display the center name with a nullish/coalescing or conditional
fallback to t('view.missingInOpenAPI').
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 560d0b30-bcdb-4e1f-a407-5d493efdac99
📒 Files selected for processing (16)
src/pages/centers/centers-view/general-tab/CentersGeneralTab.tsxsrc/pages/clients/Clients.tsxsrc/pages/clients/clients-view/ClientsView.tsxsrc/pages/clients/clients-view/Identities-tab/ClientsidentitiesTab.tsxsrc/pages/clients/clients-view/address-tab/ClientsAddressTab.tsxsrc/pages/clients/clients-view/charges/view-charge/ViewCharge.tsxsrc/pages/clients/clients-view/family-members-tab/ClientsFamilyMembersAddTab.tsxsrc/pages/clients/clients-view/family-members-tab/ClientsFamilyMembersTab.tsxsrc/pages/clients/clients-view/general-tab/ClientsGeneralTab.tsxsrc/pages/groups/groups-view/GroupsView.tsxsrc/pages/groups/groups-view/general-tab/GroupsGeneralTab.tsxsrc/pages/groups/groups-view/group-actions/AttachMeeting.tsxsrc/pages/groups/groups-view/group-actions/EditGroups.tsxsrc/pages/groups/groups-view/group-actions/ManageGroupMembers.tsxsrc/pages/groups/groups-view/group-actions/TransferClients.tsxsrc/pages/login/Login.tsx
✅ Files skipped from review due to trivial changes (2)
- src/pages/groups/groups-view/group-actions/AttachMeeting.tsx
- src/pages/groups/groups-view/group-actions/ManageGroupMembers.tsx
🚧 Files skipped from review as they are similar to previous changes (10)
- src/pages/groups/groups-view/group-actions/EditGroups.tsx
- src/pages/clients/clients-view/general-tab/ClientsGeneralTab.tsx
- src/pages/clients/clients-view/family-members-tab/ClientsFamilyMembersTab.tsx
- src/pages/login/Login.tsx
- src/pages/clients/Clients.tsx
- src/pages/clients/clients-view/ClientsView.tsx
- src/pages/clients/clients-view/Identities-tab/ClientsidentitiesTab.tsx
- src/pages/groups/groups-view/general-tab/GroupsGeneralTab.tsx
- src/pages/groups/groups-view/group-actions/TransferClients.tsx
- src/pages/clients/clients-view/charges/view-charge/ViewCharge.tsx
Comment on lines
+96
to
+99
| opts.push({ | ||
| label: t('view.menu.edit'), | ||
| path: `groups/${group?.id}/edit`, | ||
| }) |
There was a problem hiding this comment.
Hide or disable menu actions until group.id is available.
These entries are created on the initial render as well, so their paths can briefly become groups/undefined/... before the fetch completes. A fast click here will navigate to an invalid route. Guard menuOptions on group?.id or keep the dropdown disabled until the record is loaded.
Suggested guard
const menuOptions = useMemo(() => {
+ if (!group?.id) {
+ return []
+ }
+
const opts: any[] = []Also applies to: 177-180
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/groups/groups-view/GroupsView.tsx` around lines 96 - 99, The menu
options are being created before group.id is available causing transient paths
like `groups/undefined/...`; update the logic that builds `menuOptions` (the
block that calls opts.push with label t('view.menu.edit') and path
`groups/${group?.id}/edit`) to only add edit/view/delete entries when
`group?.id` is truthy, or alternatively keep the dropdown component disabled
until the record loads (e.g., use the same group loading boolean used by
GroupsView to gate the menu rendering); ensure you apply the same guard to the
other pushes around lines where `groups/${group?.id}/...` are created so no menu
item is produced with an undefined id.
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.
Description
This change ensures that when the password visibility is toggled on the login page via the eye icon, the cursor remains at the end of the text and the input field remains focused.
Previously, changing the input type between password and text caused the browser to lose the selection state and focus, resetting the cursor to the beginning of the field. By using a React useRef and a setTimeout, we programmatically restore focus and set the selection range to the end of the string immediately after the DOM update.
Dependencies:
No new dependencies were added.
Demo
Summary by CodeRabbit
New Features
Style