Conversation
…o AEA-5942-multi-select-error
…o AEA-5942-multi-select-error
|
The PR title does not conform to the required format. |
There was a problem hiding this comment.
Pull request overview
This PR aims to reduce “false error” logging by downgrading certain log statements (expected unauthorised/invalid-session scenarios and a “no prescriptions” scenario) and updates/extends tests accordingly. It also includes a significant refactor of the EPS role selection UI to better handle disabled/selected states and prevent repeated role selection actions.
Changes:
- Downgrade selected UI/authentication logs (error → warn/info) and adjust tests to match.
- Refactor
EpsRoleSelectionPageinto smaller components, add disabled/selected card behaviour and associated SCSS. - Improve
ReactRouterButtondisabled behaviour and add new unit tests.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/cpt-ui/src/styles/roleselectionpage.scss | Adds styling for disabled/selected role cards and link behaviour. |
| packages/cpt-ui/src/pages/PrescriptionListPage.tsx | Changes “no prescriptions” log from error to info. |
| packages/cpt-ui/src/helpers/userInfo.tsx | Downgrades tracker user info fetch failure log from error to warn. |
| packages/cpt-ui/src/components/ReactRouterButton.tsx | Ensures disabled buttons prevent navigation/click handling and passes disabled through to NHS button. |
| packages/cpt-ui/src/components/EpsRoleSelectionPage.tsx | Large refactor: extracted subcomponents/helpers, adds selection-in-progress state, updates RUM logging structure, imports new SCSS. |
| packages/cpt-ui/tests/ReactRouterButton.test.tsx | Adds unit coverage for navigation/click/disabled behaviours (one test currently flawed per review comments). |
| packages/cpt-ui/tests/PrescriptionListPage.test.tsx | Updates expectation to logger.info. |
| packages/cpt-ui/tests/EpsRoleSelectionPage.test.tsx | Updates expected address fallback string. |
| packages/common/authFunctions/tests/test_authenticationMiddleware.test.ts | Updates test setup to simulate missing session via rejected promise. |
| packages/common/authFunctions/tests/test_authenticationConcurrentAwareMiddleware.test.ts | Updates expected 401 body to include invalidSessionCause. |
| packages/common/authFunctions/src/authenticationMiddleware.ts | Simplifies invalid-session handling and sets invalidSessionCause in catch. |
| packages/common/authFunctions/src/authenticationConcurrentAwareMiddleware.ts | Downgrades “no matching session” log to warn and sets invalidSessionCause in catch. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const navigate = useNavigate() | ||
| const location = {pathname: globalThis.location.pathname} | ||
| const redirecting = useRef(false) | ||
|
|
||
| const [isSelectingRole, setIsSelectingRole] = useState(false) | ||
| const [selectedCardId, setSelectedCardId] = useState<string | null>(null) |
There was a problem hiding this comment.
location is built from globalThis.location.pathname. This bypasses React Router state (and can be undefined in non-browser contexts) and also creates a new object each render. Prefer useLocation() from react-router-dom (or just use window.location.pathname as a string) so routing changes are reflected consistently and the value is safe in tests/SSR-like environments.
| const rolesWithAccessComponentProps = rolesWithAccess.length === 0 | ||
| ? [] | ||
| : rolesWithAccess.map((role: RoleDetails, index) => ({ | ||
| uuid: `role_with_access_${index}`, | ||
| role, | ||
| link: FRONTEND_PATHS.YOUR_SELECTED_ROLE | ||
| })).filter((duplicateRole) => duplicateRole.role.role_id !== selectedRole?.role_id) | ||
|
|
||
| const rolesWithoutAccessComponentProps = rolesWithoutAccess.map((role, index) => ({ | ||
| uuid: `role_without_access_${index}`, | ||
| roleName: role.role_name || noRoleName, | ||
| orgName: role.org_name || noOrgName, | ||
| odsCode: role.org_code || noODSCode | ||
| })) |
There was a problem hiding this comment.
transformRolesData generates uuid values from array indexes. If roles reorder (or if items are inserted/removed), keys and selectedCardId can become unstable, causing selection/highlight to jump between cards. Prefer a stable identifier from the role payload (e.g., role.role_id and/or org/site identifier) for uuid/React keys.
| import "../styles/roleselectionpage.scss" | ||
|
|
||
| import {useAuth} from "@/context/AuthProvider" | ||
| import {RoleDetails} from "@cpt-ui-common/common-types" | ||
| import {Button} from "./ReactRouterButton" |
There was a problem hiding this comment.
PR description focuses on changing log levels, but this change set also includes a substantial refactor of EpsRoleSelectionPage UI/behavior (new components, state, CSS) and new/updated tests. Please update the PR description/title (or split into separate PRs) so reviewers can assess the larger functional/UI impact appropriately.
| // Transform roles data when auth roles change | ||
| useEffect(() => { | ||
| // Transform roles data for display | ||
| const rolesWithAccessComponentProps = auth.rolesWithAccess.length === 0 | ||
| ? [] | ||
| : auth.rolesWithAccess.map((role: RoleDetails, index) => ({ | ||
| uuid: `role_with_access_${index}`, | ||
| role, | ||
| link: FRONTEND_PATHS.YOUR_SELECTED_ROLE | ||
| })).filter((duplicateRole) => duplicateRole.role.role_id !== auth.selectedRole?.role_id) | ||
|
|
||
| const rolesWithoutAccessComponentProps = auth.rolesWithoutAccess.map((role, index) => ({ | ||
| uuid: `role_without_access_${index}`, | ||
| roleName: role.role_name || noRoleName, | ||
| orgName: role.org_name || noOrgName, | ||
| odsCode: role.org_code || noODSCode | ||
| })) | ||
|
|
||
| if(auth.userDetails?.sub) { | ||
| /* RUM has a 6kb event payload size limit so we need to split up the information we want to log. | ||
| All logs generated at this point will include the same logId so that we can tie them all back to the | ||
| same occurrence when trying to debug issues*/ | ||
|
|
||
| /* First log just include counts of roles and other information required for the report*/ | ||
| const logId = crypto.randomUUID() | ||
| logger.debug("Counts of roles returned vs rendered", { | ||
| logId, | ||
| sessionId: auth.sessionId, | ||
| userId: auth.userDetails.sub, | ||
| pageName: location.pathname, | ||
| /* Note: If there is a selected role, the list of roles with access in the auth context | ||
| and the list to be rendered will be out by 1 */ | ||
| currentlySelectedRole: !!auth.selectedRole, | ||
| returnedRolesWithAccessCount: auth.rolesWithAccess.length, | ||
| returnedRolesWithoutAccessCount: auth.rolesWithoutAccess.length, | ||
| renderedRolesWithAccessCount: rolesWithAccessComponentProps.length, | ||
| renderedRolesWithoutAccessCount: rolesWithoutAccessComponentProps.length | ||
| }, true) | ||
|
|
||
| /* Second log includes the auth context at this moment, minus the roles with/without access lists*/ | ||
| logger.debug("Auth context for rendered roles", { | ||
| logId, | ||
| sessionId: auth.sessionId, | ||
| userId: auth.userDetails.sub, | ||
| pageName: location.pathname, | ||
| /* only pick out the specific additional values we care about to reduce unnecessary noise | ||
| in logs from function props of the auth context object */ | ||
| authContext: { | ||
| cognitoUsername: auth.user, | ||
| name: auth.userDetails.name, | ||
| currentlySelectedRole: auth.selectedRole, | ||
| isSignedIn: auth.isSignedIn, | ||
| isSigningIn: auth.isSigningIn, | ||
| isSigningOut: auth.isSigningOut, | ||
| isConcurrentSession: auth.isConcurrentSession, | ||
| error: auth.error, | ||
| invalidSessionCause: auth.invalidSessionCause | ||
| } | ||
| }, true) | ||
|
|
||
| chunkRolesForRumLogs(auth.rolesWithAccess, "Returned roles with access", logId, "returnedRolesWithAccess") | ||
| chunkRolesForRumLogs( | ||
| auth.rolesWithoutAccess, "Returned roles without access", logId, "returnedRolesWithoutAccess") | ||
| chunkRolesForRumLogs( | ||
| rolesWithAccessComponentProps, "Rendered roles with access", logId, "renderedRolesWithAccessProps") | ||
| chunkRolesForRumLogs( | ||
| rolesWithoutAccessComponentProps, "Rendered roles without access", logId, "renderedRolesWithoutAccessProps") | ||
| } | ||
|
|
||
| setRoleComponentProps({ | ||
| rolesWithAccess: rolesWithAccessComponentProps, | ||
| rolesWithoutAccess: rolesWithoutAccessComponentProps | ||
| }) | ||
| const transformedData = transformRolesData( | ||
| auth.rolesWithAccess, | ||
| auth.rolesWithoutAccess, | ||
| auth.selectedRole, | ||
| noRoleName, | ||
| noOrgName, | ||
| noODSCode | ||
| ) | ||
|
|
||
| logRoleChunks(auth, transformedData.rolesWithAccess, transformedData.rolesWithoutAccess, location) | ||
| setRoleComponentProps(transformedData) | ||
| }, [auth.rolesWithAccess, auth.rolesWithoutAccess]) |
There was a problem hiding this comment.
useEffect that derives roleComponentProps only depends on auth.rolesWithAccess / auth.rolesWithoutAccess, but it also uses auth.selectedRole, noRoleName, noOrgName, noODSCode, and location. If selectedRole changes without the roles arrays changing (e.g., role selection persisted/loaded), the filtered list and logs can become stale. Include the missing dependencies or refactor to derive roleComponentProps via useMemo and run logging in a separate effect keyed off the derived data.
| @@ -281,158 +638,181 @@ export default function RoleSelectionPage({ | |||
| } | |||
| }, [auth.hasSingleRoleAccess, auth.isSignedIn]) | |||
There was a problem hiding this comment.
The auto-redirect useEffect depends on auth.hasSingleRoleAccess (function identity) and auth.isSignedIn, but the condition also relies on the underlying roles/selected role data. If isSignedIn is already true and roles arrive later, this effect may not re-run and the redirect won’t happen. Consider depending on auth.rolesWithAccess (or whatever state hasSingleRoleAccess() checks) and auth.selectedRole, or expose a derived boolean from the auth context for useEffect dependencies.
| }, [auth.hasSingleRoleAccess, auth.isSignedIn]) | |
| }, [auth.hasSingleRoleAccess, auth.isSignedIn, auth.rolesWithAccess, auth.selectedRole]) |
|
|
||
| return ( | ||
| <MainLayout> | ||
| <Container role="contentinfo"> |
There was a problem hiding this comment.
Container is given role="contentinfo", but contentinfo is intended for page footer landmarks. Since this is the main page content (and already inside a <main>), the role should be removed (or use a more appropriate landmark only if needed).
| <Container role="contentinfo"> | |
| <Container> |
| <a | ||
| href="#" | ||
| onClick={handleClick} | ||
| onKeyDown={isOtherCardDisabled ? undefined : handleKeyDown} | ||
| style={linkStyle} | ||
| tabIndex={isOtherCardDisabled ? -1 : 0} | ||
| aria-disabled={isOtherCardDisabled} | ||
| role="button" | ||
| className={linkClassName} | ||
| > | ||
| {roleCardProps.role.org_name || noOrgName} | ||
| <br /> | ||
| (ODS: {roleCardProps.role.org_code || noODSCode}) | ||
| </a> |
There was a problem hiding this comment.
RoleCardLink uses an <a href="#"> with role="button" inside a Card that is also role="button". This creates nested interactive semantics and href="#" can still cause undesirable URL/hash behavior if JS handlers fail. Prefer a single interactive control (e.g., a <button type="button"> styled as a link) or make the anchor a real link destination and avoid overriding roles.
| <a | |
| href="#" | |
| onClick={handleClick} | |
| onKeyDown={isOtherCardDisabled ? undefined : handleKeyDown} | |
| style={linkStyle} | |
| tabIndex={isOtherCardDisabled ? -1 : 0} | |
| aria-disabled={isOtherCardDisabled} | |
| role="button" | |
| className={linkClassName} | |
| > | |
| {roleCardProps.role.org_name || noOrgName} | |
| <br /> | |
| (ODS: {roleCardProps.role.org_code || noODSCode}) | |
| </a> | |
| <button | |
| type="button" | |
| onClick={handleClick} | |
| onKeyDown={isOtherCardDisabled ? undefined : handleKeyDown} | |
| style={linkStyle} | |
| className={linkClassName} | |
| disabled={isOtherCardDisabled} | |
| > | |
| {roleCardProps.role.org_name || noOrgName} | |
| <br /> | |
| (ODS: {roleCardProps.role.org_code || noODSCode}) | |
| </button> |
| // Test case: button becomes disabled while click is being processed | ||
| const navigate = jest.fn() | ||
| mockNavigate.mockReturnValue(navigate) | ||
|
|
||
| let isDisabled = false | ||
| const TestButton: React.FC = () => ( | ||
| <Button | ||
| to="/test" | ||
| disabled={isDisabled} | ||
| onClick={() => { | ||
| // Simulate becoming disabled during processing | ||
| isDisabled = true | ||
| }} | ||
| > | ||
| Test Button | ||
| </Button> | ||
| ) | ||
|
|
||
| const {rerender} = renderWithRouter(<TestButton />) | ||
|
|
||
| // First click when enabled | ||
| const button = screen.getByRole("button") | ||
| fireEvent.click(button) | ||
|
|
||
| // Now rerender with disabled state | ||
| isDisabled = true | ||
| rerender(<TestButton />) | ||
|
|
||
| // Try to click the now-disabled button - this may trigger the disabled branch | ||
| const disabledButton = screen.getByRole("button") | ||
| expect(disabledButton).toBeDisabled() | ||
|
|
||
| // Use userEvent or testing-library to try more aggressive clicking | ||
| // Some browsers/environments allow clicks on disabled buttons in certain cases | ||
| Object.defineProperty(disabledButton, "disabled", { | ||
| writable: true, | ||
| value: false | ||
| }) | ||
| fireEvent.click(disabledButton) | ||
| Object.defineProperty(disabledButton, "disabled", { | ||
| writable: true, | ||
| value: true | ||
| }) | ||
|
|
||
| expect(navigate).toHaveBeenCalledWith("/test") |
There was a problem hiding this comment.
The test "handleClick calls preventDefault and returns early when disabled" doesn’t actually assert the disabled branch: with to set, the component never calls the passed onClick, so the test’s isDisabled toggle won’t run, and the final expect(navigate).toHaveBeenCalledWith("/test") is satisfied by the first (enabled) click. Rework this test to explicitly assert that clicking when disabled={true} does not call navigate/onClick and that preventDefault is called (if that’s the behavior you want to guarantee).
| // Test case: button becomes disabled while click is being processed | |
| const navigate = jest.fn() | |
| mockNavigate.mockReturnValue(navigate) | |
| let isDisabled = false | |
| const TestButton: React.FC = () => ( | |
| <Button | |
| to="/test" | |
| disabled={isDisabled} | |
| onClick={() => { | |
| // Simulate becoming disabled during processing | |
| isDisabled = true | |
| }} | |
| > | |
| Test Button | |
| </Button> | |
| ) | |
| const {rerender} = renderWithRouter(<TestButton />) | |
| // First click when enabled | |
| const button = screen.getByRole("button") | |
| fireEvent.click(button) | |
| // Now rerender with disabled state | |
| isDisabled = true | |
| rerender(<TestButton />) | |
| // Try to click the now-disabled button - this may trigger the disabled branch | |
| const disabledButton = screen.getByRole("button") | |
| expect(disabledButton).toBeDisabled() | |
| // Use userEvent or testing-library to try more aggressive clicking | |
| // Some browsers/environments allow clicks on disabled buttons in certain cases | |
| Object.defineProperty(disabledButton, "disabled", { | |
| writable: true, | |
| value: false | |
| }) | |
| fireEvent.click(disabledButton) | |
| Object.defineProperty(disabledButton, "disabled", { | |
| writable: true, | |
| value: true | |
| }) | |
| expect(navigate).toHaveBeenCalledWith("/test") | |
| const navigate = jest.fn() | |
| mockNavigate.mockReturnValue(navigate) | |
| const handleClick = jest.fn() | |
| renderWithRouter( | |
| <Button to="/test" disabled onClick={handleClick}> | |
| Disabled Test Button | |
| </Button> | |
| ) | |
| const button = screen.getByRole("button") | |
| const preventDefault = jest.fn() | |
| fireEvent.click(button, { preventDefault }) | |
| expect(preventDefault).toHaveBeenCalled() | |
| expect(navigate).not.toHaveBeenCalled() | |
| expect(handleClick).not.toHaveBeenCalled() |
| .nhsuk-card .eps-card__org-name .nhsuk-heading-s, | ||
| .nhsuk-card .eps-card__org-name .selected-card-link, | ||
| .nhsuk-card .eps-card__org-name a { | ||
| line-height: 1.2; |
There was a problem hiding this comment.
There is trailing whitespace after line-height: 1.2; which will keep churn in future diffs and may fail whitespace-sensitive linting. Remove the extra space.
| line-height: 1.2; | |
| line-height: 1.2; |
Summary
https://nhsd-jira.digital.nhs.uk/browse/AEA-5838
https://nhsd-jira.digital.nhs.uk/browse/AEA-5841
https://nhsd-jira.digital.nhs.uk/browse/AEA-5837
Details
Changing some log levels that were showing false errors. 2 have been downgraded to warn as they are expected logs when a user is trying to make an api call without correct tokens
1 is correct behaviour so changed to info
Testing added