Skip to content

Feature/is 11176/integrate lwa repo#106

Merged
aleixsuau merged 12 commits intointegration/IS-5161/login-web-appfrom
feature/IS-11176/integrate-lwa-repo
Mar 31, 2026
Merged

Feature/is 11176/integrate lwa repo#106
aleixsuau merged 12 commits intointegration/IS-5161/login-web-appfrom
feature/IS-11176/integrate-lwa-repo

Conversation

@aleixsuau
Copy link
Copy Markdown

@aleixsuau aleixsuau commented Mar 26, 2026

This PR syncs Vitest version with the main monorepo, fixes lint errors, and improves React hook correctness

@aleixsuau aleixsuau changed the base branch from main to integration/IS-5161/login-web-app March 26, 2026 13:38
@aleixsuau aleixsuau marked this pull request as ready for review March 26, 2026 13:48
@luisgoncalves
Copy link
Copy Markdown
Contributor

Looks like there are some small formatting issues, also. Could you please run Prettier and update?

let history = await screen.findByTestId('history');
let historyData = JSON.parse(history.textContent ?? '[]') as HaapiStepperHistoryEntry[];
// eslint-disable-next-line @typescript-eslint/no-unnecessary-type-assertion
let historyData = JSON.parse(history.textContent!) as HaapiStepperHistoryEntry[];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Locally, I see screen.findByTestId returning HTMLElement (non-nullable), for which textContent is never null, so this shouldn't be needed. Did I miss something?

Same applies to the similar occurrences below.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The linting passes when removing the non-null operator (!), but the build fails:
Screenshot 2026-03-27 at 13 19 58

textContent can be null (https://developer.mozilla.org/en-US/docs/Web/API/Node/textContent), so I'm not sure why the linting does not get it right. Since it's a test typing issue, I didn't want to spend more time on it. I refactored it now to make it cleaner.

export const HaapiStepperFormContext = createContext<HaapiStepperFormContextValue | null>(null);

export function useHaapiStepperForm(): HaapiStepperFormContextValue {
// eslint-disable-next-line @eslint-react/no-use-context -- useContext is preferred here over use() to keep explicit null handling
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Locally I see the same return type with useContext and use. It's the context value itself that may be null.

I think we can go with use. Did I miss something?

Copy link
Copy Markdown
Author

@aleixsuau aleixsuau Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure, I'd like to understand the feature better first. It seems that use  slightly changes error handling and default null  values: https://www.saschb2b.com/blog/use-hook-react

I'll refactor it later, probably including suspense and adapting the error handling to it. I added a task for this in Jira.

@aleixsuau aleixsuau requested a review from Copilot March 31, 2026 11:05
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR integrates updates aligned with the main monorepo by syncing Vitest, fixing lint issues, and improving React hook correctness/memoization in the HAAPI stepper components.

Changes:

  • Sync Vitest dependency range and update lockfile contents accordingly.
  • Improve HAAPI stepper runtime behavior (error notifier dismissal logic, cleanup on unmount, memoized context values).
  • Address lint/a11y correctness in tests/components (e.g., button type="button", hook-related lint adjustments) and add new tests for the error notifier.

Reviewed changes

Copilot reviewed 12 out of 13 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/login-web-app/src/shared/feature/error-handling/ErrorBoundary.spec.tsx Adds explicit button types in test components to prevent unintended form-submit behavior.
src/login-web-app/src/haapi-stepper/feature/steps/HaapiStepperStepUI.spec.tsx Removes unnecessary lint disables and adds a hooks lint suppression for a test component.
src/login-web-app/src/haapi-stepper/feature/stepper/HaapiStepperErrorNotifier.tsx Refactors notifier to track dismissed errors and simplifies render logic.
src/login-web-app/src/haapi-stepper/feature/stepper/HaapiStepperErrorNotifier.spec.tsx Adds coverage for notifier rendering, dismissal, and timing behavior.
src/login-web-app/src/haapi-stepper/feature/stepper/HaapiStepper.tsx Adds unmount cleanup and memoizes context value; adjusts hook deps linting.
src/login-web-app/src/haapi-stepper/feature/stepper/HaapiStepper.spec.tsx Refactors history parsing helper and updates lint suppressions.
src/login-web-app/src/haapi-stepper/feature/actions/form/HaapiStepperFormContext.ts Adds eslint suppression around useContext usage.
src/login-web-app/src/haapi-stepper/feature/actions/form/HaapiStepperForm.tsx Memoizes context value and stabilizes submit with useCallback.
src/login-web-app/src/haapi-stepper/feature/actions/form/HaapiStepperForm.spec.tsx Adds a hooks lint suppression for a test component.
src/login-web-app/src/haapi-stepper/data-access/haapi-fetch-utils.spec.ts Updates assertion style (toThrowErrortoThrow).
src/login-web-app/package.json Pins Vitest to ~4.0.6 to match monorepo expectations.
package-lock.json Updates resolved Vitest-related packages per the new semver range.
.github/workflows/lwa-github-ci-workflow.yml Adds a dedicated CI workflow for the login web app, including Slack notifications.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

</div>

{notificationError.dataHelpers.messages.length && (
{notificationMessages.length && (
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

notificationMessages.length && (...) will render 0 into the DOM when there are no messages (React renders numbers). Use an explicit boolean check like notificationMessages.length > 0 (or notificationMessages.length !== 0) to avoid accidentally rendering a 0 text node.

Suggested change
{notificationMessages.length && (
{notificationMessages.length > 0 && (

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done ✅

Comment on lines 1140 to 1146
useEffect(() => {
if (field.type === HAAPI_FORM_FIELDS.USERNAME) {
formState.set(field, prefilledUsernameValue);
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [field]);

Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The // eslint-disable-next-line react-hooks/exhaustive-deps comment is placed inside the useEffect callback, so it likely won’t suppress the react-hooks/exhaustive-deps warning (the rule reports on the useEffect(...) call expression). Move the disable comment directly above the useEffect( line (or use eslint-disable-line on the hook call) so linting is actually suppressed.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incorrect ❌

Comment on lines 264 to 270
useEffect(() => {
if (field.type === HAAPI_FORM_FIELDS.USERNAME) {
formState.set(field, prefilledUsernameValue);
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [field]);

Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The // eslint-disable-next-line react-hooks/exhaustive-deps comment is inside the useEffect callback, which usually won’t disable the react-hooks/exhaustive-deps warning (it reports on the hook call). Place the disable comment on the line immediately above useEffect( (or use eslint-disable-line on that call) to ensure lint is actually suppressed.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incorrect ❌


const startButton = await screen.findByRole('button', { name: 'Start BankID' });

Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There appears to be stray whitespace on the blank line after the findByRole call (line contains spaces only). Please remove trailing whitespace to avoid failing formatting / whitespace lint rules and to keep diffs clean.

Suggested change

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done ✅

const { error } = useHaapiStepper();
const [notificationError, setNotificationError] = useState<HaapiStepperAppError | HaapiStepperInputError | null>();
const [isNotificationVisible, setIsNotificationVisible] = useState(false);
const currentError = error?.app ?? (showInputErrorNotifications ? error?.input : null);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor comment, wasn't sure what currentError referred to when reading this, is it the error on the current step? or what does current refer to?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aleixsuau aleixsuau merged commit 784d1ca into integration/IS-5161/login-web-app Mar 31, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants