test(journey-client): ported legacy Journey tests to new Client #474
Conversation
|
WalkthroughAdds a centralized callback renderer (e2e/journey-app/callback-map.ts), 21 journey UI component renderers and an index/README, integrates rendering into the main journey app, expands E2E Playwright and OIDC tests, updates async-events with redirect-aware helpers, and adjusts a few package exports and a middleware types change. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Journey App
participant Callbacks as renderCallbacks
participant Map as callback-map
participant Components as Component Renderers
participant DOM as Browser DOM
App->>Callbacks: renderCallbacks(journeyEl, callbacks[])
loop each callback
Callbacks->>Map: renderCallback(journeyEl, callback, idx)
Map->>Map: callback.getType()
Map->>Components: dispatch to specific renderer
Components->>DOM: create elements & attach listeners
end
Note over DOM: user interacts → events trigger callback updates
sequenceDiagram
participant Test as Playwright Test
participant Async as asyncEvents
participant Page as Browser Page
participant App as Auth App
participant Provider as Auth Provider
Test->>Async: clickWithRedirect(buttonText, expectedUrl)
Async->>Page: click(button) and waitForURL(expectedUrl) in parallel
Page->>Provider: request /authenticate
Provider-->>Page: redirect to expectedUrl
Page-->>Async: navigation resolved
Async-->>Test: returns after redirect
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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 |
|
View your CI Pipeline Execution ↗ for commit b02ef84
☁️ Nx Cloud last updated this comment at |
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e/journey-app/main.ts (1)
84-88: Restore journey start options after logout
Afterterminate()you calljourneyClient.start()with no arguments, but the initial start required thejourneyandnoSessionparameters derived from the URL. Losing those options means any flow that depends on them (e.g., custom tree selection or forcing no-session) will immediately break once the user logs out and tries again. Please persist and reuse the original start options.- let step = await journeyClient.start({ - journey: searchParams.get('journey') || '', - query: { noSession: searchParams.get('no-session') || 'false' }, - }); + const startOptions = { + journey: searchParams.get('journey') || '', + query: { noSession: searchParams.get('no-session') || 'false' }, + }; + let step = await journeyClient.start(startOptions); @@ - step = await journeyClient.start(); + step = await journeyClient.start(startOptions);
🧹 Nitpick comments (22)
e2e/journey-suites/src/otp-register.ts (3)
12-12: Improve test description specificity.The description "Test happy paths on test page" is generic and doesn't clearly indicate this test covers OTP registration.
Consider:
-test('Test happy paths on test page', async ({ page }) => { +test('OTP registration journey completes successfully', async ({ page }) => {
19-22: Remove unnecessary Promise.resolve from console event handler.Console event handlers are synchronous callbacks and don't require returning a promise.
Apply this diff:
page.on('console', async (msg) => { messageArray.push(msg.text()); - return Promise.resolve(true); });
37-37: Strengthen assertion specificity.Checking for the generic string 'otp' is vague and prone to false positives.
Consider asserting on a more specific message fragment that confirms OTP registration behavior.
e2e/journey-suites/src/email-suspend.test.ts (2)
12-12: Improve test description specificity.The description "Test happy paths on test page" is generic and doesn't clearly indicate this test covers email suspend functionality.
Consider:
-test('Test happy paths on test page', async ({ page }) => { +test('Login with email suspend displays confirmation message', async ({ page }) => {
16-22: Remove unused messageArray or add assertions.The
messageArrayis populated from console events but never used in any assertions. Either remove it or add assertions that validate console output.If not needed:
- const messageArray: string[] = []; - - // Listen for events on page - page.on('console', async (msg) => { - messageArray.push(msg.text()); - return Promise.resolve(true); - });e2e/journey-app/components/choice.ts (1)
35-41: Consider explicit null checks for DOM operations.The optional chaining on
journeyEl?.appendChild()andjourneyEl?.querySelector()will silently fail ifjourneyElis null. In a test/demo environment this might be acceptable, but explicit guards would make debugging easier.Example:
if (!journeyEl) { console.warn('journeyEl is null, cannot render choice component'); return; } journeyEl.appendChild(label); journeyEl.appendChild(select); const selectElement = journeyEl.querySelector(`#${collectorKey}`); if (selectElement) { selectElement.addEventListener('change', (event) => { const selectedIndex = Number((event.target as HTMLSelectElement).value); callback.setChoiceIndex(selectedIndex); }); }e2e/journey-suites/src/registration.test.ts (2)
12-12: Improve test description specificity.The description "Test happy paths on test page" is generic and doesn't clearly indicate this test covers user registration.
Consider:
-test('Test happy paths on test page', async ({ page }) => { +test('User registration journey with security questions completes successfully', async ({ page }) => {
21-24: Remove unnecessary Promise.resolve from console event handler.Console event handlers are synchronous callbacks and don't require returning a promise.
Apply this diff:
page.on('console', async (msg) => { messageArray.push(msg.text()); - return Promise.resolve(true); });e2e/journey-app/components/ping-protect-initialize.ts (1)
22-22: Track TODO for PingOne Protect initialization.The TODO indicates this is a placeholder implementation. Ensure the PingOne Protect module initialization is tracked and implemented when needed.
Do you want me to open a new issue to track the PingOne Protect initialization implementation?
e2e/journey-suites/src/request-middleware.test.ts (2)
21-24: Remove unnecessary Promise.resolve from console event handler.Console event handlers are synchronous callbacks and don't require returning a promise.
Apply this diff:
page.on('console', async (msg) => { messageArray.push(msg.text()); - return Promise.resolve(true); });
26-34: Combine duplicate request event handlers.Two separate
page.on('request')handlers could be merged into a single handler for better performance and maintainability.Apply this diff:
- page.on('request', async (req) => { - networkArray.push(`${new URL(req.url()).pathname}, ${req.resourceType()}`); - }); - - page.on('request', async (req) => { - const headers = req.headers(); - - headerArray.push(new Headers(headers)); - }); + page.on('request', async (req) => { + networkArray.push(`${new URL(req.url()).pathname}, ${req.resourceType()}`); + const headers = req.headers(); + headerArray.push(new Headers(headers)); + });e2e/journey-app/components/attribute-input.ts (1)
41-50: Consider CSS selector escaping for special characters.If
collectorKeycontains special characters (e.g., brackets, colons), the querySelector will fail. While unlikely with typical callback input names, you could useCSS.escape()for robustness or query the input directly via reference.Alternative approach using direct reference:
- journeyEl?.appendChild(label); - journeyEl?.appendChild(input); + journeyEl?.appendChild(label); + journeyEl?.appendChild(input); - journeyEl?.querySelector(`#${collectorKey}`)?.addEventListener('input', (event) => { + input.addEventListener('input', (event) => { const target = event.target as HTMLInputElement; if (attributeType === 'BooleanAttributeInputCallback') { callback.setInputValue(target.checked); } else if (attributeType === 'NumberAttributeInputCallback') { callback.setInputValue(Number(target.value)); } else { callback.setInputValue(target.value); } });e2e/journey-app/components/kba-create.ts (1)
23-24: Consider template literals for string concatenation.While the string concatenation works correctly, template literals would be more idiomatic and easier to maintain.
- questionLabel.innerText = callback.getPrompt() + ' ' + idx; + questionLabel.innerText = `${callback.getPrompt()} ${idx}`;Similarly for line 40:
- answerLabel.innerText = 'Answer ' + idx + ':'; + answerLabel.innerText = `Answer ${idx}:`;e2e/journey-suites/src/choice-confirm-poll.test.ts (1)
12-12: Consider a more descriptive test name.The test description "Test happy paths on test page" is generic. Consider something more specific like "Should complete login journey with choice, confirmation, and polling callbacks" to better describe what this test validates.
e2e/journey-suites/src/no-session.test.ts (1)
12-12: Consider a more descriptive test name.The test description "Test happy paths on test page" is generic. Consider something like "Should complete login journey with no-session parameter and log session token status" to better describe the test's purpose.
e2e/journey-suites/src/protect.test.ts (1)
12-12: Test is currently skipped.This test is disabled with
test.skip. Ensure there's tracking (e.g., a JIRA ticket or TODO) for when this should be enabled. Additionally, consider a more descriptive test name like "Should complete Protect journey with data collection and verification" instead of the generic "Test happy paths on test page".e2e/journey-app/components/select-idp.ts (1)
46-47: Optional chaining is unnecessary on non-nullable parameter.Since
journeyElis typed asHTMLDivElement(line 10), the optional chaining (journeyEl?.appendChild) is unnecessary. You can safely usejourneyEl.appendChild(...)directly.Apply this diff to remove unnecessary optional chaining:
- journeyEl?.appendChild(label); - journeyEl?.appendChild(container); + journeyEl.appendChild(label); + journeyEl.appendChild(container);e2e/journey-suites/src/custom-paths.test.ts (1)
20-23: Remove unnecessary Promise.resolve.The explicit
return Promise.resolve(true)is unnecessary. Thepage.on('console')event handler doesn't use the return value, and even if it did, the function is already async so returning a value directly would suffice.Apply this diff to simplify:
page.on('console', async (msg) => { messageArray.push(msg.text()); - return Promise.resolve(true); });e2e/journey-app/components/recaptcha.ts (1)
37-42: Placeholder implementation - reCAPTCHA integration incomplete.The component renders a placeholder but doesn't integrate the actual reCAPTCHA widget. The callback's
getSiteKey()method is available but unused, andsetResult()is never called.Do you want me to generate a complete reCAPTCHA integration implementation, or should this remain a placeholder for e2e testing purposes?
e2e/journey-suites/src/device-profile.test.ts (1)
19-22: Unnecessary Promise wrapper in event listener.The console event listener returns
Promise.resolve(true), but this return value is not meaningful for this event handler.Simplify the event listener:
- page.on('console', async (msg) => { + page.on('console', (msg) => { messageArray.push(msg.text()); - return Promise.resolve(true); });e2e/journey-app/components/device-profile.ts (1)
24-31: Simplify placeholder setTimeout.The setTimeout only contains a console.log wrapped in an unnecessary try-catch. If this is placeholder code, simplify it. If device profile collection should happen here, implement it or add a TODO.
Simplify to:
setTimeout(() => { - try { - // Device profile collection is typically handled automatically by the callback - console.log('Device profile collection initiated'); - } catch (error) { - console.error('Device profile collection failed:', error); - } + console.log('Device profile collection initiated'); }, 100);Or if real collection logic is needed, add a TODO comment explaining what needs to be implemented.
e2e/journey-app/components/recaptcha-enterprise.ts (1)
38-43: Track TODO for reCAPTCHA Enterprise integration.The component currently provides only a placeholder UI. The TODO comment indicates that real reCAPTCHA Enterprise script loading and initialization is needed.
Would you like me to open an issue to track the implementation of the reCAPTCHA Enterprise integration?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (49)
e2e/journey-app/callback-map.ts(1 hunks)e2e/journey-app/components/README.md(1 hunks)e2e/journey-app/components/attribute-input.ts(1 hunks)e2e/journey-app/components/choice.ts(1 hunks)e2e/journey-app/components/confirmation.ts(1 hunks)e2e/journey-app/components/device-profile.ts(1 hunks)e2e/journey-app/components/hidden-value.ts(1 hunks)e2e/journey-app/components/index.ts(1 hunks)e2e/journey-app/components/kba-create.ts(1 hunks)e2e/journey-app/components/metadata.ts(1 hunks)e2e/journey-app/components/ping-protect-evaluation.ts(1 hunks)e2e/journey-app/components/ping-protect-initialize.ts(1 hunks)e2e/journey-app/components/polling-wait.ts(1 hunks)e2e/journey-app/components/recaptcha-enterprise.ts(1 hunks)e2e/journey-app/components/recaptcha.ts(1 hunks)e2e/journey-app/components/redirect.ts(1 hunks)e2e/journey-app/components/select-idp.ts(1 hunks)e2e/journey-app/components/suspended-text-output.ts(1 hunks)e2e/journey-app/components/terms-and-conditions.ts(1 hunks)e2e/journey-app/components/text-input.ts(1 hunks)e2e/journey-app/components/text-output.ts(1 hunks)e2e/journey-app/components/validated-password.ts(1 hunks)e2e/journey-app/components/validated-username.ts(1 hunks)e2e/journey-app/main.ts(6 hunks)e2e/journey-app/tsconfig.app.json(1 hunks)e2e/journey-suites/src/choice-confirm-poll.test.ts(1 hunks)e2e/journey-suites/src/custom-paths.test.ts(1 hunks)e2e/journey-suites/src/device-profile.test.ts(1 hunks)e2e/journey-suites/src/email-suspend.test.ts(1 hunks)e2e/journey-suites/src/login.test.ts(2 hunks)e2e/journey-suites/src/no-session.test.ts(1 hunks)e2e/journey-suites/src/otp-register.ts(1 hunks)e2e/journey-suites/src/protect.test.ts(1 hunks)e2e/journey-suites/src/registration.test.ts(1 hunks)e2e/journey-suites/src/request-middleware.test.ts(1 hunks)e2e/oidc-app/src/ping-am/index.html(1 hunks)e2e/oidc-app/src/ping-one/index.html(1 hunks)e2e/oidc-app/src/utils/oidc-app.ts(1 hunks)e2e/oidc-suites/src/login.spec.ts(4 hunks)e2e/oidc-suites/src/logout.spec.ts(3 hunks)e2e/oidc-suites/src/token.spec.ts(6 hunks)e2e/oidc-suites/src/user.spec.ts(1 hunks)e2e/oidc-suites/src/utils/async-events.ts(2 hunks)packages/journey-client/src/index.ts(1 hunks)packages/journey-client/src/lib/client.store.test.ts(1 hunks)packages/journey-client/src/lib/client.store.ts(1 hunks)packages/journey-client/src/types.ts(2 hunks)packages/sdk-effects/sdk-request-middleware/src/lib/request-mware.test.ts(2 hunks)packages/sdk-effects/sdk-request-middleware/src/lib/request-mware.types.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: ryanbas21
Repo: ForgeRock/ping-javascript-sdk PR: 430
File: packages/journey-client/src/lib/callbacks/name-callback.ts:9-15
Timestamp: 2025-10-22T15:14:00.044Z
Learning: In packages/journey-client, callback classes are internal implementation details not part of the public API. The callbacks barrel (src/lib/callbacks/index.ts) intentionally only exports the base JourneyCallback class. Internal code imports concrete callback classes directly from their individual files (e.g., factory.ts, journey-client.ts).
📚 Learning: 2025-10-22T15:14:00.044Z
Learnt from: ryanbas21
Repo: ForgeRock/ping-javascript-sdk PR: 430
File: packages/journey-client/src/lib/callbacks/name-callback.ts:9-15
Timestamp: 2025-10-22T15:14:00.044Z
Learning: In packages/journey-client, callback classes are internal implementation details not part of the public API. The callbacks barrel (src/lib/callbacks/index.ts) intentionally only exports the base JourneyCallback class. Internal code imports concrete callback classes directly from their individual files (e.g., factory.ts, journey-client.ts).
Applied to files:
packages/journey-client/src/index.tse2e/journey-app/callback-map.tse2e/journey-app/components/terms-and-conditions.tse2e/journey-app/components/README.mde2e/journey-app/components/suspended-text-output.tse2e/journey-app/components/confirmation.tse2e/journey-app/components/redirect.tse2e/journey-app/components/text-output.tse2e/journey-app/components/index.tspackages/journey-client/src/lib/client.store.tspackages/journey-client/src/types.tse2e/journey-app/components/recaptcha.tse2e/journey-app/tsconfig.app.jsonpackages/journey-client/src/lib/client.store.test.tse2e/journey-app/components/recaptcha-enterprise.tse2e/journey-app/main.ts
📚 Learning: 2025-10-27T17:55:33.855Z
Learnt from: cerebrl
Repo: ForgeRock/ping-javascript-sdk PR: 430
File: e2e/journey-app/main.ts:83-87
Timestamp: 2025-10-27T17:55:33.855Z
Learning: In the e2e/journey-app and similar e2e test applications in this repository, XSS hardening and similar production security practices are not required since these are exclusively test/development applications.
Applied to files:
e2e/journey-app/components/README.mde2e/journey-suites/src/protect.test.tse2e/journey-suites/src/no-session.test.ts
🧬 Code graph analysis (36)
e2e/journey-suites/src/choice-confirm-poll.test.ts (1)
e2e/oidc-suites/src/utils/async-events.ts (3)
asyncEvents(7-76)navigate(54-56)clickButton(9-18)
e2e/journey-app/components/choice.ts (1)
packages/journey-client/src/lib/callbacks/choice-callback.ts (1)
ChoiceCallback(14-64)
e2e/journey-app/callback-map.ts (16)
packages/journey-client/src/lib/callbacks/base-callback.ts (1)
BaseCallback(13-101)e2e/journey-app/components/attribute-input.ts (1)
attributeInputComponent(9-51)e2e/journey-app/components/choice.ts (1)
choiceComponent(9-42)e2e/journey-app/components/confirmation.ts (1)
confirmationComponent(9-57)e2e/journey-app/components/hidden-value.ts (1)
hiddenValueComponent(9-26)e2e/journey-app/components/kba-create.ts (1)
kbaCreateComponent(9-62)e2e/journey-app/components/metadata.ts (1)
metadataComponent(9-27)e2e/journey-app/components/ping-protect-evaluation.ts (1)
pingProtectEvaluationComponent(9-23)e2e/journey-app/components/ping-protect-initialize.ts (1)
pingProtectInitializeComponent(9-23)e2e/journey-app/components/polling-wait.ts (1)
pollingWaitComponent(9-48)e2e/journey-app/components/recaptcha.ts (1)
recaptchaComponent(9-42)e2e/journey-app/components/recaptcha-enterprise.ts (1)
recaptchaEnterpriseComponent(9-43)e2e/journey-app/components/redirect.ts (1)
redirectComponent(9-45)e2e/journey-app/components/select-idp.ts (1)
selectIdpComponent(9-56)e2e/journey-app/components/suspended-text-output.ts (1)
suspendedTextOutputComponent(9-34)e2e/journey-app/components/terms-and-conditions.ts (1)
termsAndConditionsComponent(9-36)
e2e/journey-app/components/attribute-input.ts (1)
packages/journey-client/src/lib/callbacks/attribute-input-callback.ts (1)
AttributeInputCallback(18-81)
e2e/journey-app/components/validated-password.ts (1)
packages/journey-client/src/lib/callbacks/validated-create-password-callback.ts (1)
ValidatedCreatePasswordCallback(14-75)
e2e/journey-app/components/device-profile.ts (1)
packages/journey-client/src/lib/callbacks/device-profile-callback.ts (1)
DeviceProfileCallback(16-51)
e2e/journey-app/components/hidden-value.ts (1)
packages/journey-client/src/lib/callbacks/hidden-value-callback.ts (1)
HiddenValueCallback(14-21)
e2e/journey-app/components/kba-create.ts (1)
packages/journey-client/src/lib/callbacks/kba-create-callback.ts (1)
KbaCreateCallback(14-61)
e2e/journey-suites/src/request-middleware.test.ts (1)
e2e/oidc-suites/src/utils/async-events.ts (3)
asyncEvents(7-76)navigate(54-56)clickButton(9-18)
e2e/journey-app/components/polling-wait.ts (1)
packages/journey-client/src/lib/callbacks/polling-wait-callback.ts (1)
PollingWaitCallback(14-35)
e2e/journey-suites/src/otp-register.ts (1)
e2e/oidc-suites/src/utils/async-events.ts (3)
asyncEvents(7-76)navigate(54-56)clickButton(9-18)
e2e/journey-app/components/terms-and-conditions.ts (1)
packages/journey-client/src/lib/callbacks/terms-and-conditions-callback.ts (1)
TermsAndConditionsCallback(14-51)
e2e/journey-suites/src/custom-paths.test.ts (1)
e2e/oidc-suites/src/utils/async-events.ts (3)
asyncEvents(7-76)navigate(54-56)clickButton(9-18)
e2e/journey-app/components/suspended-text-output.ts (1)
packages/journey-client/src/lib/callbacks/suspended-text-output-callback.ts (1)
SuspendedTextOutputCallback(14-21)
e2e/oidc-suites/src/logout.spec.ts (2)
e2e/oidc-suites/src/utils/async-events.ts (4)
asyncEvents(7-76)navigate(54-56)clickWithRedirect(19-23)clickButton(9-18)e2e/oidc-suites/src/utils/demo-users.ts (4)
pingAmUsername(10-10)pingAmPassword(11-11)pingOneUsername(12-12)pingOnePassword(13-13)
e2e/journey-suites/src/protect.test.ts (1)
e2e/oidc-suites/src/utils/async-events.ts (3)
asyncEvents(7-76)navigate(54-56)clickButton(9-18)
e2e/journey-app/components/confirmation.ts (1)
packages/journey-client/src/lib/callbacks/confirmation-callback.ts (1)
ConfirmationCallback(14-78)
e2e/journey-suites/src/email-suspend.test.ts (1)
e2e/oidc-suites/src/utils/async-events.ts (3)
asyncEvents(7-76)navigate(54-56)clickButton(9-18)
e2e/journey-app/components/metadata.ts (1)
packages/journey-client/src/lib/callbacks/metadata-callback.ts (1)
MetadataCallback(14-28)
e2e/journey-app/components/select-idp.ts (1)
packages/journey-client/src/lib/callbacks/select-idp-callback.ts (1)
SelectIdPCallback(21-46)
e2e/oidc-suites/src/user.spec.ts (2)
e2e/oidc-suites/src/utils/async-events.ts (4)
asyncEvents(7-76)navigate(54-56)clickWithRedirect(19-23)clickButton(9-18)e2e/oidc-suites/src/utils/demo-users.ts (4)
pingAmUsername(10-10)pingAmPassword(11-11)pingOneUsername(12-12)pingOnePassword(13-13)
e2e/journey-app/components/redirect.ts (1)
packages/journey-client/src/lib/callbacks/redirect-callback.ts (1)
RedirectCallback(14-28)
e2e/journey-app/components/ping-protect-initialize.ts (1)
packages/journey-client/src/lib/callbacks/ping-protect-initialize-callback.ts (1)
PingOneProtectInitializeCallback(14-44)
e2e/journey-app/components/text-output.ts (2)
e2e/journey-app/components/text-input.ts (1)
textComponent(9-34)packages/journey-client/src/lib/callbacks/text-output-callback.ts (1)
TextOutputCallback(14-36)
e2e/journey-app/components/text-input.ts (3)
e2e/journey-app/components/text-output.ts (1)
textComponent(9-24)packages/journey-client/src/lib/callbacks/name-callback.ts (1)
NameCallback(14-35)packages/journey-client/src/lib/callbacks/text-input-callback.ts (1)
TextInputCallback(14-35)
e2e/journey-app/components/validated-username.ts (1)
packages/journey-client/src/lib/callbacks/validated-create-username-callback.ts (1)
ValidatedCreateUsernameCallback(15-76)
e2e/journey-app/components/recaptcha.ts (1)
packages/journey-client/src/lib/callbacks/recaptcha-callback.ts (1)
ReCaptchaCallback(14-35)
e2e/journey-suites/src/registration.test.ts (1)
e2e/oidc-suites/src/utils/async-events.ts (3)
asyncEvents(7-76)navigate(54-56)clickButton(9-18)
e2e/journey-suites/src/device-profile.test.ts (1)
e2e/oidc-suites/src/utils/async-events.ts (3)
asyncEvents(7-76)navigate(54-56)clickButton(9-18)
e2e/journey-suites/src/login.test.ts (1)
e2e/oidc-suites/src/utils/async-events.ts (3)
asyncEvents(7-76)navigate(54-56)clickButton(9-18)
e2e/journey-app/components/recaptcha-enterprise.ts (1)
packages/journey-client/src/lib/callbacks/recaptcha-enterprise-callback.ts (1)
ReCaptchaEnterpriseCallback(18-72)
e2e/journey-suites/src/no-session.test.ts (1)
e2e/oidc-suites/src/utils/async-events.ts (3)
asyncEvents(7-76)navigate(54-56)clickButton(9-18)
e2e/journey-app/main.ts (2)
packages/sdk-effects/sdk-request-middleware/src/lib/request-mware.types.ts (1)
RequestMiddleware(17-21)e2e/journey-app/callback-map.ts (1)
renderCallbacks(153-157)
e2e/journey-app/components/ping-protect-evaluation.ts (1)
packages/journey-client/src/lib/callbacks/ping-protect-evaluation-callback.ts (1)
PingOneProtectEvaluationCallback(14-47)
e2e/oidc-suites/src/login.spec.ts (1)
e2e/oidc-suites/src/utils/async-events.ts (3)
asyncEvents(7-76)navigate(54-56)clickWithRedirect(19-23)
e2e/oidc-suites/src/token.spec.ts (2)
e2e/oidc-suites/src/utils/async-events.ts (4)
asyncEvents(7-76)navigate(54-56)clickWithRedirect(19-23)clickButton(9-18)e2e/oidc-suites/src/utils/demo-users.ts (4)
pingAmUsername(10-10)pingAmPassword(11-11)pingOneUsername(12-12)pingOnePassword(13-13)
🪛 ast-grep (0.39.7)
e2e/journey-app/main.ts
[warning] 99-101: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: errorEl.innerHTML = <pre id="errorMessage">${error}</pre>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 99-101: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: errorEl.innerHTML = <pre id="errorMessage">${error}</pre>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
⏰ 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). (2)
- GitHub Check: pr
- GitHub Check: Mend Code Security Check
🔇 Additional comments (28)
packages/journey-client/src/types.ts (1)
15-15: LGTM: Callback exports are consistent with established pattern.These new exports align with the existing public API surface in types.ts, which already exposes multiple callback types. The additions enable downstream UI components to reference
ConfirmationCallbackandTextInputCallbacktypes.Also applies to: 31-31
e2e/journey-suites/src/request-middleware.test.ts (1)
12-12: Investigate why this test is skipped.The test is marked with
test.skip. Ensure there's a tracking issue or plan to enable this middleware test.Is there a known issue preventing this test from running, or is it pending implementation of the mock middleware?
e2e/journey-app/components/attribute-input.ts (1)
14-14: LGTM! Safe key derivation with fallback.The optional chaining with fallback pattern ensures a valid identifier is always generated.
packages/journey-client/src/lib/client.store.test.ts (1)
13-13: LGTM! Import path updated to reflect module rename.The import path change from
./journey.store.jsto./client.store.jsaligns with the module refactoring. Test logic remains unchanged.e2e/journey-app/tsconfig.app.json (1)
12-12: LGTM! TypeScript configuration updated for new module.Adding
callback-map.tsto the include array ensures the new callback rendering dispatcher is compiled as part of the project.packages/sdk-effects/sdk-request-middleware/src/lib/request-mware.test.ts (1)
29-29: LGTM! Test inputs updated to match required headers field.The addition of
headers: new Headers()to test inputs correctly aligns with the updatedModifiedFetchArgsinterface whereheadersis now a required field.Also applies to: 40-40, 51-51, 77-77
packages/journey-client/src/index.ts (1)
8-8: LGTM! Public export source updated to client store.The re-export path change from
./lib/journey.store.jsto./lib/client.store.jsreflects the module refactoring. The public API surface remains unchanged; only the internal source module differs.packages/sdk-effects/sdk-request-middleware/src/lib/request-mware.types.ts (1)
39-39: LGTM! Headers field now required for better type safety.Making
headersa required field inModifiedFetchArgsstrengthens the type contract and ensures middleware can always rely on aHeadersobject being present. The corresponding test updates confirm all usages have been updated accordingly.e2e/oidc-app/src/ping-am/index.html (1)
18-19: Event handler verified—no issues found.The button with id
get-tokens-backgroundis properly wired ate2e/oidc-app/src/utils/oidc-app.ts:106with a corresponding click event listener that correctly invokes the background token retrieval flow usingoidcClient.token.get({ backgroundRenew: true }).e2e/journey-app/components/kba-create.ts (2)
55-57: Fix incorrect type assertion for select element.Line 56 casts the select element's event target to
HTMLInputElement, which is semantically incorrect. It should be cast toHTMLSelectElement.Apply this diff:
questionInput.addEventListener('input', (event) => { - callback.setQuestion((event.target as HTMLInputElement).value); + callback.setQuestion((event.target as HTMLSelectElement).value); });Likely an incorrect or invalid review comment.
30-35: Add a placeholder/default option to handle empty predefined questions.The select dropdown will have no options if
getPredefinedQuestions()returns an empty array, potentially blocking test progression. Consider adding a default placeholder option before the forEach loop, similar to the pattern ine2e/journey-app/components/select-idp.ts. Alternatively, verify that the Journey configuration always provides at least one predefined question.packages/journey-client/src/lib/client.store.ts (1)
14-14: LGTM!The import path update from
journey.store.utils.jstoclient.store.utils.jsis consistent with the module refactoring mentioned in the AI summary. The change is straightforward and properly reflected in related files.e2e/oidc-app/src/ping-one/index.html (1)
18-19: LGTM!The button label clarification and new background token retrieval button align well with the updated token renewal logic in
oidc-app.ts. The distinction between local and background operations is now clear in the UI.e2e/oidc-app/src/utils/oidc-app.ts (1)
106-114: LGTM!The separation of token retrieval functionality into two distinct handlers is well-designed:
get-tokens-background: Background renewal without forcingrenew-tokens: Background renewal with force flagThis provides clearer testing capabilities for different token renewal scenarios.
e2e/journey-suites/src/choice-confirm-poll.test.ts (1)
47-47: Hardcoded timeout is acceptable for e2e polling simulation.The 1500ms timeout appropriately simulates the async polling behavior for test purposes. This is a reasonable approach for e2e testing.
e2e/journey-suites/src/no-session.test.ts (1)
34-34: Assertion pattern is correct.Using
await expect(...).toBe(true)withmessageArray.includes()is appropriate here. While theawaitisn't strictly necessary for this synchronous assertion, it maintains consistency with Playwright's expect API and doesn't cause any issues.e2e/journey-app/components/README.md (1)
1-84: Excellent documentation!This README provides comprehensive coverage of the component system with:
- Clear categorization of component types
- Consistent pattern definition with TypeScript examples
- Usage examples and parameter descriptions
- Implementation notes for developers
The documentation will be valuable for understanding and extending the journey callback component system.
e2e/journey-app/components/select-idp.ts (1)
9-56: LGTM! Component follows the established pattern.The SelectIdP component correctly:
- Derives a collectorKey from the callback payload with a fallback
- Creates a select dropdown with a default disabled placeholder
- Populates options from
callback.getProviders()- Wires the change event to call
callback.setProvider()The implementation aligns well with the component pattern documented in the README.
e2e/journey-app/components/hidden-value.ts (1)
9-26: LGTM!The hidden value component is correctly implemented with proper fallbacks and optional chaining.
e2e/journey-app/components/confirmation.ts (1)
9-57: LGTM!The confirmation component correctly renders radio options with proper default selection and event handling to update the callback state.
e2e/journey-app/components/redirect.ts (1)
9-45: LGTM!The redirect component correctly implements the external provider redirect flow with proper error handling.
e2e/journey-suites/src/device-profile.test.ts (1)
12-12: Skipped test may indicate incomplete work.The test is marked with
test.skip, which prevents it from running. This typically indicates incomplete implementation or a known issue.Please verify whether this test is intentionally disabled or if there's a tracking issue for enabling it.
e2e/oidc-suites/src/utils/async-events.ts (2)
19-23: Good addition for redirect-based flows.The new
clickWithRedirectmethod appropriately handles navigation waits alongside click actions, which is useful for OAuth redirect flows.
28-30: Improved response validation.Adding
response.ok()checks inclickLinkandpressEnterprovides better validation that requests succeeded.Also applies to: 61-63
e2e/journey-app/components/suspended-text-output.ts (1)
9-34: Component implementation looks correct.The suspended text output component follows the established pattern for callback components, properly derives the collector key, and provides appropriate visual styling for a suspended state message.
e2e/journey-suites/src/login.test.ts (1)
13-14: Good refactoring to use test helpers.The test now properly uses the
clickButtonhelper for form submission and logout actions, which improves reliability by waiting for the expected response before proceeding.Also applies to: 27-27, 32-32
e2e/journey-app/components/validated-password.ts (1)
8-29: Component implementation is correct.The validated password component follows the established pattern, properly wires the input event to the callback's
setPasswordmethod, and uses appropriate type casting.e2e/journey-app/components/validated-username.ts (1)
7-11: Type update looks correct.The component properly updates the import and function signature to use
ValidatedCreateUsernameCallback, aligning with the callback type system.
| const attributeType = callback.getType(); | ||
| if (attributeType === 'BooleanAttributeInputCallback') { | ||
| input.type = 'checkbox'; | ||
| input.checked = (callback.getInputValue() as boolean) || false; | ||
| } else if (attributeType === 'NumberAttributeInputCallback') { | ||
| input.type = 'number'; | ||
| input.value = String(callback.getInputValue() || ''); | ||
| } else { | ||
| input.type = 'text'; | ||
| input.value = String(callback.getInputValue() || ''); | ||
| } |
There was a problem hiding this comment.
Handle undefined values from getInputValue().
The type assertion and String conversion don't explicitly handle undefined returns from callback.getInputValue(). This could lead to input.checked = undefined (falsy) for checkboxes or input.value = "undefined" (string) for text/number inputs.
Apply this diff to handle undefined explicitly:
const attributeType = callback.getType();
if (attributeType === 'BooleanAttributeInputCallback') {
input.type = 'checkbox';
- input.checked = (callback.getInputValue() as boolean) || false;
+ input.checked = Boolean(callback.getInputValue());
} else if (attributeType === 'NumberAttributeInputCallback') {
input.type = 'number';
- input.value = String(callback.getInputValue() || '');
+ const numValue = callback.getInputValue();
+ input.value = numValue !== undefined ? String(numValue) : '';
} else {
input.type = 'text';
- input.value = String(callback.getInputValue() || '');
+ const textValue = callback.getInputValue();
+ input.value = textValue !== undefined ? String(textValue) : '';
}📝 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.
| const attributeType = callback.getType(); | |
| if (attributeType === 'BooleanAttributeInputCallback') { | |
| input.type = 'checkbox'; | |
| input.checked = (callback.getInputValue() as boolean) || false; | |
| } else if (attributeType === 'NumberAttributeInputCallback') { | |
| input.type = 'number'; | |
| input.value = String(callback.getInputValue() || ''); | |
| } else { | |
| input.type = 'text'; | |
| input.value = String(callback.getInputValue() || ''); | |
| } | |
| const attributeType = callback.getType(); | |
| if (attributeType === 'BooleanAttributeInputCallback') { | |
| input.type = 'checkbox'; | |
| input.checked = Boolean(callback.getInputValue()); | |
| } else if (attributeType === 'NumberAttributeInputCallback') { | |
| input.type = 'number'; | |
| const numValue = callback.getInputValue(); | |
| input.value = numValue !== undefined ? String(numValue) : ''; | |
| } else { | |
| input.type = 'text'; | |
| const textValue = callback.getInputValue(); | |
| input.value = textValue !== undefined ? String(textValue) : ''; | |
| } |
🤖 Prompt for AI Agents
In e2e/journey-app/components/attribute-input.ts around lines 22 to 32,
getInputValue() can return undefined and the current type assertions/String
conversions produce undesired results (checkbox can get undefined, inputs can
become the string "undefined"); change the assignments to explicitly handle
undefined: read the value once into a local const, for
BooleanAttributeInputCallback set input.checked = Boolean(value) (or
input.checked = !!value) to ensure a true/false, and for
NumberAttributeInputCallback and text fallback set input.value = value ===
undefined ? '' : String(value) (or use value ?? ''), so undefined never becomes
the string "undefined" and checkboxes always get a boolean.
| // Add CSS animation | ||
| const style = document.createElement('style'); | ||
| style.textContent = ` | ||
| @keyframes spin { | ||
| 0% { transform: rotate(0deg); } | ||
| 100% { transform: rotate(360deg); } | ||
| } | ||
| `; | ||
| document.head.appendChild(style); |
There was a problem hiding this comment.
Prevent duplicate style injection.
The @keyframes style is appended to document.head every time this component is rendered. If the component is rendered multiple times (e.g., multiple polling callbacks in a journey), duplicate style elements will accumulate in the document head.
Apply this diff to check for an existing style before adding:
// Add CSS animation
- const style = document.createElement('style');
- style.textContent = `
- @keyframes spin {
- 0% { transform: rotate(0deg); }
- 100% { transform: rotate(360deg); }
- }
- `;
- document.head.appendChild(style);
+ if (!document.getElementById('polling-wait-spin-keyframes')) {
+ const style = document.createElement('style');
+ style.id = 'polling-wait-spin-keyframes';
+ style.textContent = `
+ @keyframes spin {
+ 0% { transform: rotate(0deg); }
+ 100% { transform: rotate(360deg); }
+ }
+ `;
+ document.head.appendChild(style);
+ }📝 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.
| // Add CSS animation | |
| const style = document.createElement('style'); | |
| style.textContent = ` | |
| @keyframes spin { | |
| 0% { transform: rotate(0deg); } | |
| 100% { transform: rotate(360deg); } | |
| } | |
| `; | |
| document.head.appendChild(style); | |
| // Add CSS animation | |
| if (!document.getElementById('polling-wait-spin-keyframes')) { | |
| const style = document.createElement('style'); | |
| style.id = 'polling-wait-spin-keyframes'; | |
| style.textContent = ` | |
| @keyframes spin { | |
| 0% { transform: rotate(0deg); } | |
| 100% { transform: rotate(360deg); } | |
| } | |
| `; | |
| document.head.appendChild(style); | |
| } |
🤖 Prompt for AI Agents
In e2e/journey-app/components/polling-wait.ts around lines 33 to 41, the code
always appends a new <style> with the same @keyframes to document.head causing
duplicate style elements; modify it to first check whether the style already
exists (e.g., query the document.head for an element with a unique id or for an
existing @keyframes rule via document.styleSheets) and only create/append the
<style> if not present, or alternatively assign a stable id to the created
<style> and skip creation when an element with that id is found.
| // Terms text area | ||
| console.log(callback.getTerms); |
There was a problem hiding this comment.
Invoke getTerms() to log the actual terms.
Line 19 logs the function reference rather than calling it, which will output something like [Function: getTerms] instead of the actual terms and conditions text.
Apply this diff:
// Terms text area
- console.log(callback.getTerms);
+ console.log(callback.getTerms());📝 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.
| // Terms text area | |
| console.log(callback.getTerms); | |
| // Terms text area | |
| console.log(callback.getTerms()); |
🤖 Prompt for AI Agents
In e2e/journey-app/components/terms-and-conditions.ts around lines 18 to 19, the
code logs the function reference callback.getTerms instead of its return value;
call the function (i.e., invoke getTerms()) when logging so the actual terms
string is printed, and ensure any async function is awaited if it returns a
promise before logging.
| if (callback.getType() === 'NameCallback') { | ||
| (callback as NameCallback).setName((event.target as HTMLInputElement).value); | ||
| } else { | ||
| (callback as TextInputCallback).setInputValue((event.target as HTMLInputElement).value); | ||
| } |
There was a problem hiding this comment.
Use the correct public API method for TextInputCallback.
Line 31 calls setInputValue directly on TextInputCallback, but the public API provides a setInput method. The setInputValue method is likely inherited from BaseCallback and may be protected or internal.
Apply this diff:
journeyEl?.querySelector(`#${collectorKey}`)?.addEventListener('input', (event) => {
if (callback.getType() === 'NameCallback') {
(callback as NameCallback).setName((event.target as HTMLInputElement).value);
} else {
- (callback as TextInputCallback).setInputValue((event.target as HTMLInputElement).value);
+ (callback as TextInputCallback).setInput((event.target as HTMLInputElement).value);
}
});📝 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.
| if (callback.getType() === 'NameCallback') { | |
| (callback as NameCallback).setName((event.target as HTMLInputElement).value); | |
| } else { | |
| (callback as TextInputCallback).setInputValue((event.target as HTMLInputElement).value); | |
| } | |
| if (callback.getType() === 'NameCallback') { | |
| (callback as NameCallback).setName((event.target as HTMLInputElement).value); | |
| } else { | |
| (callback as TextInputCallback).setInput((event.target as HTMLInputElement).value); | |
| } |
🤖 Prompt for AI Agents
In e2e/journey-app/components/text-input.ts around lines 28 to 32, the code
calls the internal/Inherited method setInputValue on TextInputCallback; replace
that call with the public API method setInput. Cast the callback to
TextInputCallback as before, read the input value from event.target as
HTMLInputElement, and pass that value into callback.setInput(...) instead of
setInputValue(...). Ensure no other logic changes are made.
| ) { | ||
| const collectorKey = callback?.payload?.input?.[0].name || `collector-${idx}`; | ||
| const message = callback.getMessage() || ''; | ||
| const p = document.createElement('paragraph'); |
There was a problem hiding this comment.
Fix invalid HTML element tag.
'paragraph' is not a valid HTML element. The correct tag is 'p'. Using an invalid tag creates an HTMLUnknownElement, which may render but is non-standard and semantically incorrect.
Apply this diff:
- const p = document.createElement('paragraph');
+ const p = document.createElement('p');📝 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.
| const p = document.createElement('paragraph'); | |
| const p = document.createElement('p'); |
🤖 Prompt for AI Agents
In e2e/journey-app/components/text-output.ts around line 16, the code creates an
invalid HTML element using document.createElement('paragraph'); change the tag
to the standard 'p' element by using document.createElement('p') so the DOM node
is a proper HTMLParagraphElement; also search nearby code for any uses or CSS
selectors tied to 'paragraph' and update them to target 'p' if present.
| if (step?.type === 'Step') { | ||
| console.log('Continuing journey to next step'); | ||
| renderForm(); | ||
| } else if (step?.type === 'LoginSuccess') { | ||
| console.log('Basic login successful'); | ||
| console.log('Journey completed successfully'); | ||
| renderComplete(); | ||
| } else if (step?.type === 'LoginFailure') { | ||
| console.error('Journey failed'); | ||
| renderForm(); | ||
| renderError(); |
There was a problem hiding this comment.
Don’t call renderForm() on a failure step
When step.type === 'LoginFailure', renderForm() immediately throws (“Expected step to be defined and of type Step”), so the failure path crashes before the user ever sees the error UI. Drop the form render (or fetch a new Step first) so the failure flow survives.
- } else if (step?.type === 'LoginFailure') {
- console.error('Journey failed');
- renderForm();
- renderError();
+ } else if (step?.type === 'LoginFailure') {
+ console.error('Journey failed');
+ renderError();📝 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.
| if (step?.type === 'Step') { | |
| console.log('Continuing journey to next step'); | |
| renderForm(); | |
| } else if (step?.type === 'LoginSuccess') { | |
| console.log('Basic login successful'); | |
| console.log('Journey completed successfully'); | |
| renderComplete(); | |
| } else if (step?.type === 'LoginFailure') { | |
| console.error('Journey failed'); | |
| renderForm(); | |
| renderError(); | |
| if (step?.type === 'Step') { | |
| console.log('Continuing journey to next step'); | |
| renderForm(); | |
| } else if (step?.type === 'LoginSuccess') { | |
| console.log('Journey completed successfully'); | |
| renderComplete(); | |
| } else if (step?.type === 'LoginFailure') { | |
| console.error('Journey failed'); | |
| renderError(); |
🤖 Prompt for AI Agents
In e2e/journey-app/main.ts around lines 148 to 157, the LoginFailure branch
calls renderForm() which throws because no Step is available; remove the
renderForm() call (or replace it with logic to fetch a new Step before
rendering) so the failure path only renders the error UI
(renderError()/renderComplete() as appropriate) and does not crash immediately
on failure.
| await page.getByLabel('Password').fill(password); | ||
| await clickButton('Submit', '/authenticate'); | ||
|
|
||
| await expect(page.getByText('Collecting device profile')).toBeVisible(); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify text content matches the component.
The test expects 'Collecting device profile', but the device-profile component (e2e/journey-app/components/device-profile.ts:18) displays 'Collecting device profile information...'.
Run the following script to confirm the actual text rendered by the component:
🏁 Script executed:
#!/bin/bash
# Check the exact text rendered by the device profile component
rg -n "innerText.*=.*'Collecting" e2e/journey-app/components/device-profile.tsLength of output: 68
Update test to match the actual component text.
The test is looking for 'Collecting device profile', but the device-profile component renders 'Collecting device profile information...'. Update line 29 to use the correct text:
await expect(page.getByText('Collecting device profile information...')).toBeVisible();
🤖 Prompt for AI Agents
In e2e/journey-suites/src/device-profile.test.ts around line 29, the test
expects the wrong text; update the assertion to match the component's actual
rendered string by replacing 'Collecting device profile' with 'Collecting device
profile information...' so the line reads an expectation for the full text
including the trailing ellipsis.
| @@ -0,0 +1,46 @@ | |||
| /* | |||
There was a problem hiding this comment.
Rename file to match naming convention.
All other test files in this directory use the .test.ts extension (e.g., email-suspend.test.ts, registration.test.ts), but this file is named otp-register.ts.
Apply this change:
-e2e/journey-suites/src/otp-register.ts
+e2e/journey-suites/src/otp-register.test.tsCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In e2e/journey-suites/src/otp-register.ts around lines 1 to 1, the filename does
not follow the repo's test naming convention (other tests use .test.ts); rename
the file to otp-register.test.ts and update any imports or references to this
file in test runner configs or index files to use the new name so the test suite
discovers and runs it.
There was a problem hiding this comment.
@ryanbas21, thank you for confirming! The file can be renamed to otp-register.test.ts when convenient to match the repository's test naming convention.
| // Fill answer with "Pizza" | ||
| await page.getByLabel('Answer 8').fill('AAA Engineering'); |
There was a problem hiding this comment.
Fix comment-code mismatch.
The comment says "Pizza" but the code fills "AAA Engineering".
Apply this diff:
- // Fill answer with "Pizza"
+ // Fill answer with "AAA Engineering"
await page.getByLabel('Answer 8').fill('AAA Engineering');📝 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.
| // Fill answer with "Pizza" | |
| await page.getByLabel('Answer 8').fill('AAA Engineering'); | |
| // Fill answer with "AAA Engineering" | |
| await page.getByLabel('Answer 8').fill('AAA Engineering'); |
🤖 Prompt for AI Agents
In e2e/journey-suites/src/registration.test.ts around lines 51-52 the inline
comment says "Fill answer with "Pizza"" but the test fills 'AAA Engineering';
update the test to match the comment by replacing the fill value with 'Pizza' so
the line reads await page.getByLabel('Answer 8').fill('Pizza'); ensuring the
comment and code are consistent.
| await clickWithRedirect('Login (Background)', '**/signon/**'); | ||
|
|
||
| await page.getByLabel('Username').fill(pingOneUsername); | ||
| await page.getByRole('textbox', { name: 'Password' }).fill(pingOnePassword); | ||
| await page.getByRole('button', { name: 'Sign On' }).click(); | ||
| await clickWithRedirect('Sign On', 'http://localhost:8443/ping-one/**'); |
There was a problem hiding this comment.
Fix PingOne redirect test to click the redirect button. Line 86 selects the background-login button, so the “redirect login” scenario actually exercises the background flow and never covers the redirect path. Please invoke the Login (Redirect) button to validate the intended journey.
Apply this diff to restore redirect coverage:
- await clickWithRedirect('Login (Background)', '**/signon/**');
+ await clickWithRedirect('Login (Redirect)', '**/signon/**');📝 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.
| await clickWithRedirect('Login (Background)', '**/signon/**'); | |
| await page.getByLabel('Username').fill(pingOneUsername); | |
| await page.getByRole('textbox', { name: 'Password' }).fill(pingOnePassword); | |
| await page.getByRole('button', { name: 'Sign On' }).click(); | |
| await clickWithRedirect('Sign On', 'http://localhost:8443/ping-one/**'); | |
| await clickWithRedirect('Login (Redirect)', '**/signon/**'); | |
| await page.getByLabel('Username').fill(pingOneUsername); | |
| await page.getByRole('textbox', { name: 'Password' }).fill(pingOnePassword); | |
| await clickWithRedirect('Sign On', 'http://localhost:8443/ping-one/**'); |
🤖 Prompt for AI Agents
In e2e/oidc-suites/src/login.spec.ts around lines 86 to 90, the test currently
clicks the background login button causing the redirect scenario to exercise the
wrong flow; change the invocation to click the "Login (Redirect)" button instead
of "Login (Background)" so the test exercises the redirect path. Update the call
to use the exact label 'Login (Redirect)' for clickWithRedirect, keep the same
redirect URL matcher ('**/signon/**') and subsequent form interactions unchanged
so the rest of the test continues to fill Username/Password and click the Sign
On redirect button.
| const responsePromise = page.waitForResponse((response) => { | ||
| return response.url().includes('/authorize') && !response.ok(); | ||
| }); | ||
| await page.getByRole('button', { name: 'Renew Tokens' }).click(); | ||
| await responsePromise; |
There was a problem hiding this comment.
any reason we are using this pattern instead of the async-events related stuff?
There was a problem hiding this comment.
Oh, at one time I had to treat this uniquely, but that's no longer the case. I'll fix it.
| it('should run all middleware testing action for no match', () => { | ||
| const runMiddleware = middlewareWrapper( | ||
| { url: new URL('https://www.example.com') }, | ||
| { url: new URL('https://www.example.com'), headers: new Headers() }, |
There was a problem hiding this comment.
believe this is the cause of the failing unit test, right?
| import { asyncEvents } from './utils/async-events.js'; | ||
| import { username, password } from './utils/demo-user.js'; | ||
|
|
||
| test.skip('Test happy paths on test page', async ({ page }) => { |
There was a problem hiding this comment.
is this a skip carried over from legacy? or was this intended? do we even need the protect test here?
There was a problem hiding this comment.
Yeah, we either need to remove this test or change what we test. I didn't want to spend too much time trying to figure it out, so I just added the skip to it for now.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
e2e/journey-suites/src/otp-register.test.ts (2)
12-12: Use a more descriptive test name.The test description "Test happy paths on test page" doesn't clearly indicate what functionality is being tested. Consider renaming to something more specific like "OTP registration journey completes successfully after login" or "Should display QR code and complete OTP registration flow".
Apply this diff:
-test('Test happy paths on test page', async ({ page }) => { +test('Should complete OTP registration journey with QR code display', async ({ page }) => {
21-21: Remove unnecessary promise return.The event handler doesn't need to return anything. The
return Promise.resolve(true)is superfluous.Apply this diff:
page.on('console', async (msg) => { messageArray.push(msg.text()); - return Promise.resolve(true); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
e2e/journey-suites/src/otp-register.test.ts(1 hunks)packages/sdk-effects/sdk-request-middleware/src/lib/request-mware.test.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/sdk-effects/sdk-request-middleware/src/lib/request-mware.test.ts
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ryanbas21
Repo: ForgeRock/ping-javascript-sdk PR: 430
File: packages/journey-client/src/lib/callbacks/name-callback.ts:9-15
Timestamp: 2025-10-22T15:14:00.044Z
Learning: In packages/journey-client, callback classes are internal implementation details not part of the public API. The callbacks barrel (src/lib/callbacks/index.ts) intentionally only exports the base JourneyCallback class. Internal code imports concrete callback classes directly from their individual files (e.g., factory.ts, journey-client.ts).
⏰ 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: pr
| expect( | ||
| messageArray.includes( | ||
| 'Scan the QR code image below with the ForgeRock Authenticator app to register your device with your login.', | ||
| ), | ||
| ).toBe(true); | ||
| expect(messageArray.includes('otp')).toBe(true); |
There was a problem hiding this comment.
Refine assertion specificity.
The assertion on Line 37 checks for the generic substring 'otp', which could match unintended console messages and lead to false positives. Consider asserting a more specific message or pattern that uniquely identifies the expected OTP-related output.
Additionally, the assertion style using .includes() wrapped in expect().toBe(true) is less idiomatic. Consider using Playwright's .toContain() for better readability:
- expect(
- messageArray.includes(
- 'Scan the QR code image below with the ForgeRock Authenticator app to register your device with your login.',
- ),
- ).toBe(true);
- expect(messageArray.includes('otp')).toBe(true);
+ expect(messageArray).toContain(
+ 'Scan the QR code image below with the ForgeRock Authenticator app to register your device with your login.',
+ );
+ expect(messageArray.some(msg => msg.includes('otp') && msg.length > 10)).toBe(true);Note: The second assertion has been modified to check for messages containing 'otp' with more than 10 characters to reduce false positives, but you should ideally match a more specific expected pattern.
📝 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.
| expect( | |
| messageArray.includes( | |
| 'Scan the QR code image below with the ForgeRock Authenticator app to register your device with your login.', | |
| ), | |
| ).toBe(true); | |
| expect(messageArray.includes('otp')).toBe(true); | |
| expect(messageArray).toContain( | |
| 'Scan the QR code image below with the ForgeRock Authenticator app to register your device with your login.', | |
| ); | |
| expect(messageArray.some(msg => msg.includes('otp') && msg.length > 10)).toBe(true); |
🤖 Prompt for AI Agents
In e2e/journey-suites/src/otp-register.test.ts around lines 32 to 37, the second
assertion is too weak and non-idiomatic: it checks for the generic substring
'otp' via messageArray.includes wrapped in expect(...).toBe(true), which can
yield false positives. Replace it with a more specific, idiomatic assertion that
both targets a unique OTP-related message and uses Playwright/Jest array matcher
semantics (e.g., assert messageArray contains a string that matches a precise
pattern like "otp:" or a full phrase such as "one-time password" or an
alphanumeric OTP token with length > 6), and use
expect(messageArray).toContain(...) or expect(...).toMatch(...) as appropriate
so the test only passes when the intended OTP output is present.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (25.00%) is below the target coverage (40.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #474 +/- ##
===========================================
+ Coverage 18.52% 67.27% +48.74%
===========================================
Files 138 86 -52
Lines 27402 6594 -20808
Branches 963 736 -227
===========================================
- Hits 5076 4436 -640
+ Misses 22326 2158 -20168
🚀 New features to boost your workflow:
|
@forgerock/davinci-client
@forgerock/oidc-client
@forgerock/protect
@forgerock/sdk-types
@forgerock/sdk-utilities
@forgerock/iframe-manager
@forgerock/sdk-logger
@forgerock/sdk-oidc
@forgerock/sdk-request-middleware
@forgerock/storage
commit: |
|
Deployed 94f0c26 to https://ForgeRock.github.io/ping-javascript-sdk/pr-474/94f0c265ff839402b52ff0db6b86aa79083d11c6 branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis🚨 Significant Changes🔻 @forgerock/journey-client - 0.0 KB (-82.0 KB, -100.0%) 📊 Minor Changes📈 @forgerock/journey-client - 82.0 KB (+0.0 KB) ➖ No Changes➖ @forgerock/device-client - 9.2 KB 13 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
JIRA Ticket
Jira ticket: SDKS-4045
Description
This ports the Journey related e2e tests from the legacy repo to this new Journey Client. This also addresses potential flakiness in the OIDC tests by refactoring the used patterns.
Summary by CodeRabbit
New Features
Documentation
Tests