Skip to content

Conversation

@ryanbas21
Copy link
Collaborator

@ryanbas21 ryanbas21 commented Dec 3, 2025

JIRA Ticket

https://pingidentity.atlassian.net/browse/SDKS-4521

Description

Adds journey client tests to e2e's. uses the mock-api to serve some data

Summary by CodeRabbit

  • New Features

    • WebAuthn registration with recovery-codes display and completion handling.
    • QR code MFA enrollment UI and QR-code step rendering.
    • Device profile collection and PingOne Protect initialization with automated collection/submission flows.
  • Tests

    • New and reactivated end-to-end tests for device profile, Protect flow, QR code journey, and recovery-codes workflow.
  • Chores

    • TypeScript project references and Protect dependency added; demo test passwords updated.

✏️ Tip: You can customize this high-level summary in your review settings.

@changeset-bot
Copy link

changeset-bot bot commented Dec 3, 2025

⚠️ No Changeset found

Latest commit: 00143a5

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@coderabbitai
Copy link

coderabbitai bot commented Dec 3, 2025

Walkthrough

Adds WebAuthn and QR-code mock responses and route branches, integrates PingOne Protect initialization/evaluation and async device-profile collection in the journey app, adds the Protect dependency and TypeScript references, and introduces new and updated Playwright e2e tests for QR, recovery-codes, Protect, and device-profile journeys.

Changes

Cohort / File(s) Summary
WebAuthn Mock Module
e2e/am-mock-api/src/app/response.webauthn.js
New module exporting webAuthnRegistrationInit, getRecoveryCodesDisplay(), and authSuccess modeling WebAuthn registration, recovery-codes display, and success payloads.
Mock Responses
e2e/am-mock-api/src/app/responses.js
Added and exported qrCodeCallbacksResponse representing a QR enrollment callback sequence (TextOutputCallback, HiddenValueCallback, ConfirmationCallback).
Auth Route Wiring
e2e/am-mock-api/src/app/routes.auth.js
Imported new mocks; added branches for TEST_WebAuthnWithRecoveryCodes, QRCodeTest, DeviceProfileCallbackTest; extended LoginPingProtect handling, safe JSON parsing, cookie/session mock handling, QR-path and recovery-codes confirmation handling, and authId-based state mutations.
Device Profile Component
e2e/journey-app/components/device-profile.ts
deviceProfileComponent now accepts idx, uses Device to async-collect profile (location/metadata flags), sets callback profile, updates UI on success/error, logs, and auto-submits on success.
Protect Initialize Component
e2e/journey-app/components/ping-protect-initialize.ts
Adds cached protectInstance, getProtectInstance() getter, accepts idx; implements full Protect init flow (config/envId, start), UI/status updates, error propagation, and auto-submit.
Protect Evaluation Component
e2e/journey-app/components/ping-protect-evaluation.ts
pingProtectEvaluationComponent accepts idx, imports getProtectInstance, auto-collects protect data via getProtectInstance().getData(), updates callback/UI, handles errors, and auto-submits on success.
QR & Recovery UI Renderers
e2e/journey-app/components/qr-code.ts
e2e/journey-app/components/recovery-codes.ts
New renderers renderQRCodeStep and renderRecoveryCodesStep that detect and render QR-code and recovery-codes steps, wire ConfirmationCallback options, and return handled status.
Main Renderer Update
e2e/journey-app/main.ts
Imports renderQRCodeStep and renderRecoveryCodesStep; attempts QR/recovery render before falling back to generic renderCallbacks.
Project Config & Dependency
e2e/journey-app/package.json
e2e/journey-app/tsconfig.app.json
e2e/journey-app/tsconfig.json
Added @forgerock/protect dependency and TypeScript project references to the protect package.
E2E Tests (updated)
e2e/journey-suites/src/device-profile.test.ts
e2e/journey-suites/src/protect.test.ts
Re-enabled and updated tests to target new journeys, added request interception to capture payloads, extended waits/timeouts and assertions to match updated UI/Protect flows.
E2E Tests (new)
e2e/journey-suites/src/qr-code.test.ts
e2e/journey-suites/src/recovery-codes.test.ts
New Playwright tests for QR enrollment and WebAuthn with recovery-codes (display, acknowledge, completion, logout, and console assertions).
Test Credentials
e2e/davinci-suites/src/utils/demo-user.ts
e2e/oidc-suites/src/utils/demo-users.ts
Updated exported demo password values.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant JourneyApp as Journey App
    participant AM as AM Mock API
    participant Callbacks as Callbacks/UI

    User->>JourneyApp: Navigate to WebAuthn journey
    JourneyApp->>AM: GET registration init
    AM-->>JourneyApp: webAuthnRegistrationInit (MetadataCallback, HiddenValueCallback)
    JourneyApp->>User: Render registration UI
    User->>JourneyApp: Submit registration (mock)
    JourneyApp->>AM: POST registration result
    AM-->>JourneyApp: getRecoveryCodesDisplay() (TextOutputCallback + ConfirmationCallback)
    JourneyApp->>User: Show recovery codes & prompt
    User->>JourneyApp: Confirm saved
    JourneyApp->>AM: POST confirmation
    AM-->>JourneyApp: authSuccess (token)
    JourneyApp->>User: Display completion
Loading
sequenceDiagram
    participant User
    participant JourneyApp as Journey App
    participant InitComp as Protect Init Component
    participant ProtectLib as Protect Library
    participant EvalComp as Protect Eval Component
    participant AM as AM Mock API

    User->>JourneyApp: Start Protect flow
    JourneyApp->>AM: GET PingOneProtectInitializeCallback
    AM-->>JourneyApp: Initialize callback
    JourneyApp->>InitComp: Render init component
    InitComp->>ProtectLib: protect.start(envId, config)
    ProtectLib-->>InitComp: instance ready
    InitComp->>JourneyApp: Auto-submit init result
    JourneyApp->>AM: GET Protect evaluation callback
    AM-->>JourneyApp: Evaluation callback
    JourneyApp->>EvalComp: Render evaluation component
    EvalComp->>ProtectLib: getData()
    ProtectLib-->>EvalComp: risk signals
    EvalComp->>JourneyApp: Auto-submit evaluation
    JourneyApp->>AM: Submit evaluation -> Success
Loading
sequenceDiagram
    participant User
    participant JourneyApp as Journey App
    participant AM as AM Mock API

    User->>JourneyApp: Start QR code journey
    JourneyApp->>AM: GET login
    AM-->>JourneyApp: qrCodeLoginResponse
    User->>JourneyApp: Submit credentials
    JourneyApp->>AM: POST login
    AM-->>JourneyApp: qrCodeCallbacksResponse (QR display, hidden URI)
    JourneyApp->>User: Show QR enrollment UI
    User->>JourneyApp: Confirm enrollment
    JourneyApp->>AM: POST confirmation
    AM-->>JourneyApp: authSuccess (token)
    JourneyApp->>User: Display completion
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Pay attention to route branching, authId/state mutations, and mock cookie/session handling in:
    • e2e/am-mock-api/src/app/routes.auth.js
  • Verify async/auto-submit and error propagation in:
    • e2e/journey-app/components/device-profile.ts
    • e2e/journey-app/components/ping-protect-initialize.ts
    • e2e/journey-app/components/ping-protect-evaluation.ts
  • Confirm new renderers and main renderer changes align with callback shapes:
    • e2e/journey-app/components/qr-code.ts
    • e2e/journey-app/components/recovery-codes.ts
    • e2e/journey-app/main.ts
  • Check Playwright tests’ request interceptions and timeouts against mock responses:
    • e2e/journey-suites/src/*

Possibly related PRs

Suggested reviewers

  • cerebrl

"I hopped with gentle paws tonight,
Mocked keys and QR codes shining bright.
Protect awoke and profiles took flight,
Recovery codes twinkled in the light.
Hooray — a hop, a test, a passing bite!" 🐇

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Title check ❓ Inconclusive The title 'Journey client tests' is vague and generic, using non-descriptive terms that don't clearly convey the scope or specifics of the changeset. Use a more specific title that describes what was added or changed, such as 'Add WebAuthn, QR code, and device profile journey tests' or 'Add end-to-end journey client tests with mock API support'.
✅ Passed checks (1 passed)
Check name Status Explanation
Description check ✅ Passed The description includes the required JIRA ticket reference and a brief explanation of changes, but lacks detail about what specific journey tests were added.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch journey-client-tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@nx-cloud
Copy link
Contributor

nx-cloud bot commented Dec 3, 2025

View your CI Pipeline Execution ↗ for commit 00143a5

Command Status Duration Result
nx run-many -t build --no-agents ✅ Succeeded <1s View ↗
nx affected -t build lint test e2e-ci ✅ Succeeded 2m 29s View ↗

☁️ Nx Cloud last updated this comment at 2025-12-10 16:51:30 UTC

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (9)
e2e/am-mock-api/src/app/response.webauthn.js (1)

167-171: Naming conflict with authSuccess in responses.js.

This file exports authSuccess with tokenId: 'webauthn-session-token', while responses.js also exports an authSuccess with tokenId: 'bar'. While routes.auth.js correctly aliases this as webAuthnSuccess, the duplicate naming could cause confusion or accidental misuse.

Consider renaming this to webAuthnAuthSuccess for clarity and to avoid potential import conflicts.

-export const authSuccess = {
-  tokenId: 'webauthn-session-token',
+export const webAuthnAuthSuccess = {
+  tokenId: 'webauthn-session-token',
   successUrl: '/console',
   realm: '/',
 };
e2e/am-mock-api/src/app/responses.js (2)

1408-1492: Code duplication with otpQRCodeCallbacks.

qrCodeCallbacksResponse (lines 1408-1492) is nearly identical to otpQRCodeCallbacks (lines 500-584), differing only in the authId. Consider reusing otpQRCodeCallbacks or extracting a factory function to reduce duplication.

// Alternative: Create a factory function
export function createQRCodeCallbacksResponse(authId) {
  return {
    authId,
    callbacks: [/* ... shared callback structure ... */],
  };
}

export const otpQRCodeCallbacks = createQRCodeCallbacksResponse('foo');
export const qrCodeCallbacksResponse = createQRCodeCallbacksResponse('qrcode-journey-confirmation');

1351-1406: Consider extracting a factory for login responses.

qrCodeLoginResponse, deviceProfileLoginResponse, and protectLoginResponse share identical structure, differing only in authId. A helper function could reduce duplication.

function createLoginResponse(authId) {
  return {
    authId,
    callbacks: [
      {
        type: 'NameCallback',
        output: [{ name: 'prompt', value: 'User Name' }],
        input: [{ name: 'IDToken1', value: '' }],
        _id: 0,
      },
      {
        type: 'PasswordCallback',
        output: [{ name: 'prompt', value: 'Password' }],
        input: [{ name: 'IDToken2', value: '' }],
        _id: 1,
      },
    ],
    stage: 'UsernamePassword',
  };
}

export const qrCodeLoginResponse = createLoginResponse('qrcode-journey-login');
export const deviceProfileLoginResponse = createLoginResponse('device-profile-journey-login');
export const protectLoginResponse = createLoginResponse('protect-journey-login');
e2e/journey-suites/src/qr-code.test.ts (1)

18-21: Unnecessary Promise.resolve(true) return.

The console event handler doesn't need to return a value. The page.on('console', ...) callback is fire-and-forget.

   page.on('console', async (msg) => {
     messageArray.push(msg.text());
-    return Promise.resolve(true);
   });
e2e/journey-app/components/ping-protect-evaluation.ts (2)

59-65: Auto-submit relies on hardcoded button ID.

The auto-submit uses document.getElementById('submitButton') which is fragile. If the button ID changes or doesn't exist, the flow silently continues without submitting. Consider logging when the button is not found.

       // Auto-submit the form after successful data collection
       setTimeout(() => {
         const submitButton = document.getElementById('submitButton') as HTMLButtonElement;
         if (submitButton) {
           submitButton.click();
+        } else {
+          console.warn('Submit button not found - auto-submit skipped');
         }
       }, 500);

27-73: Consider documenting the timeout delays.

The setTimeout at line 28 (100ms) and line 60 (500ms) use magic numbers. Adding brief comments explaining why these delays are necessary would improve maintainability.

-  // Automatically trigger Protect data collection
-  setTimeout(async () => {
+  // Automatically trigger Protect data collection after brief delay
+  // to ensure UI has finished rendering the message element
+  setTimeout(async () => {
e2e/journey-app/components/device-profile.ts (1)

27-62: Consider removing nested setTimeout for better maintainability.

The current implementation uses nested setTimeout calls with arbitrary delays (100ms and 500ms). While this works for e2e testing, it could be simplified by removing the outer setTimeout and relying on the natural async flow.

Consider this refactor:

-  // Automatically trigger device profile collection
-  setTimeout(async () => {
+  // Automatically trigger device profile collection
+  (async () => {
     try {
       const isLocationRequired = callback.isLocationRequired();
       const isMetadataRequired = callback.isMetadataRequired();
 
       console.log('Collecting device profile...', { isLocationRequired, isMetadataRequired });
 
       // Create device instance and collect profile
       const device = new Device();
       const profile = await device.getProfile({
         location: isLocationRequired,
         metadata: isMetadataRequired,
       });
 
       console.log('Device profile collected successfully');
 
       // Set the profile on the callback
       callback.setProfile(profile);
       message.innerText = 'Device profile collected successfully!';
       message.style.color = 'green';
 
-      // Auto-submit the form after successful collection
-      setTimeout(() => {
+      // Auto-submit the form after successful collection
+      await new Promise(resolve => setTimeout(resolve, 500));
-        const submitButton = document.getElementById('submitButton') as HTMLButtonElement;
-        if (submitButton) {
-          submitButton.click();
-        }
-      }, 500);
+      const submitButton = document.getElementById('submitButton') as HTMLButtonElement;
+      if (submitButton) {
+        submitButton.click();
+      }
     } catch (error) {
       console.error('Device profile collection failed:', error);
       const errorMessage = error instanceof Error ? error.message : 'Unknown error';
       message.innerText = `Collection failed: ${errorMessage}`;
       message.style.color = 'red';
     }
-  }, 100);
+  })();
e2e/journey-suites/src/protect.test.ts (1)

12-23: Use the navigate helper for consistency.

The test uses page.goto directly instead of the navigate helper from asyncEvents, which is inconsistent with other tests in this suite (e.g., device-profile.test.ts line 22). The navigate helper ensures consistent waitUntil behavior across tests.

Apply this diff:

-test('Test PingOne Protect journey flow', async ({ page }) => {
-  const { clickButton } = asyncEvents(page);
+test('Test PingOne Protect journey flow', async ({ page }) => {
+  const { clickButton, navigate } = asyncEvents(page);
 
   const messageArray: string[] = [];
 
   page.on('console', async (msg) => {
     messageArray.push(msg.text());
     return Promise.resolve(true);
   });
 
-  await page.goto('/?journey=TEST_LoginPingProtect&clientId=basic', { waitUntil: 'load' });
+  await navigate('/?journey=TEST_LoginPingProtect&clientId=basic');
e2e/journey-app/components/ping-protect-initialize.ts (1)

38-91: Consider simplifying nested setTimeout structure.

Similar to the device-profile component, the nested setTimeout calls with arbitrary delays (100ms and 500ms) could be simplified for better maintainability.

Consider this refactor:

-  // Automatically trigger Protect initialization
-  setTimeout(async () => {
+  // Automatically trigger Protect initialization
+  (async () => {
     try {
       // Get configuration from callback
       const config = callback.getConfig();
       console.log('Protect callback config:', config);
 
       if (!config?.envId) {
         const error = 'Missing envId in Protect configuration';
         console.error(error);
         callback.setClientError(error);
         message.innerText = `Initialization failed: ${error}`;
         message.style.color = 'red';
         return;
       }
 
       console.log('Initializing Protect with envId:', config.envId);
 
       // Create and store protect instance
       protectInstance = protect({ envId: config.envId });
       console.log('Protect instance created');
 
       // Initialize the Protect SDK
       console.log('Calling protect.start()...');
       const result = await protectInstance.start();
       console.log('protect.start() result:', result);
 
       if (result?.error) {
         console.error('Error initializing Protect:', result.error);
         callback.setClientError(result.error);
         message.innerText = `Initialization failed: ${result.error}`;
         message.style.color = 'red';
         return;
       }
 
       console.log('Protect initialized successfully - no errors');
       message.innerText = 'PingOne Protect initialized successfully!';
       message.style.color = 'green';
 
-      // Auto-submit the form after successful initialization
-      setTimeout(() => {
+      // Auto-submit the form after successful initialization
+      await new Promise(resolve => setTimeout(resolve, 500));
-        const submitButton = document.getElementById('submitButton') as HTMLButtonElement;
-        if (submitButton) {
-          submitButton.click();
-        }
-      }, 500);
+      const submitButton = document.getElementById('submitButton') as HTMLButtonElement;
+      if (submitButton) {
+        submitButton.click();
+      }
     } catch (error) {
       console.error('Protect initialization failed:', error);
       const errorMessage = error instanceof Error ? error.message : 'Unknown error';
       callback.setClientError(errorMessage);
       message.innerText = `Initialization failed: ${errorMessage}`;
       message.style.color = 'red';
     }
-  }, 100);
+  })();
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b6d3630 and 7edf5c0.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (13)
  • e2e/am-mock-api/src/app/response.webauthn.js (1 hunks)
  • e2e/am-mock-api/src/app/responses.js (1 hunks)
  • e2e/am-mock-api/src/app/routes.auth.js (5 hunks)
  • e2e/journey-app/components/device-profile.ts (2 hunks)
  • e2e/journey-app/components/ping-protect-evaluation.ts (2 hunks)
  • e2e/journey-app/components/ping-protect-initialize.ts (2 hunks)
  • e2e/journey-app/package.json (1 hunks)
  • e2e/journey-app/tsconfig.app.json (1 hunks)
  • e2e/journey-app/tsconfig.json (1 hunks)
  • e2e/journey-suites/src/device-profile.test.ts (1 hunks)
  • e2e/journey-suites/src/protect.test.ts (1 hunks)
  • e2e/journey-suites/src/qr-code.test.ts (1 hunks)
  • e2e/journey-suites/src/recovery-codes.test.ts (1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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:

  • e2e/am-mock-api/src/app/responses.js
📚 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-suites/src/protect.test.ts
🧬 Code graph analysis (6)
e2e/journey-suites/src/qr-code.test.ts (1)
e2e/am-mock-api/src/app/routes.auth.js (1)
  • username (137-137)
e2e/am-mock-api/src/app/response.webauthn.js (1)
e2e/am-mock-api/src/app/responses.js (2)
  • authSuccess (45-49)
  • authSuccess (45-49)
e2e/journey-app/components/ping-protect-initialize.ts (1)
packages/protect/src/lib/protect.ts (1)
  • protect (30-96)
e2e/am-mock-api/src/app/routes.auth.js (2)
e2e/am-mock-api/src/app/responses.js (20)
  • protectInitializeResponse (1523-1578)
  • protectInitializeResponse (1523-1578)
  • selectIdPCallback (439-485)
  • selectIdPCallback (439-485)
  • MetadataMarketPlaceInitialize (1205-1250)
  • MetadataMarketPlaceInitialize (1205-1250)
  • qrCodeLoginResponse (1351-1368)
  • qrCodeLoginResponse (1351-1368)
  • deviceProfileLoginResponse (1370-1387)
  • deviceProfileLoginResponse (1370-1387)
  • protectLoginResponse (1389-1406)
  • protectLoginResponse (1389-1406)
  • protectEvaluateResponse (1580-1603)
  • protectEvaluateResponse (1580-1603)
  • deviceProfileCallbackResponse (1494-1521)
  • deviceProfileCallbackResponse (1494-1521)
  • qrCodeCallbacksResponse (1408-1492)
  • qrCodeCallbacksResponse (1408-1492)
  • authSuccess (45-49)
  • authSuccess (45-49)
e2e/am-mock-api/src/app/response.webauthn.js (4)
  • webAuthnRegistrationInit (15-65)
  • webAuthnRegistrationInit (15-65)
  • authSuccess (167-171)
  • authSuccess (167-171)
e2e/journey-suites/src/device-profile.test.ts (2)
e2e/oidc-suites/src/utils/async-events.ts (3)
  • asyncEvents (7-76)
  • navigate (54-56)
  • clickButton (9-18)
e2e/journey-suites/src/utils/demo-user.ts (2)
  • username (7-7)
  • password (8-8)
e2e/journey-suites/src/protect.test.ts (3)
e2e/oidc-suites/src/utils/async-events.ts (2)
  • asyncEvents (7-76)
  • clickButton (9-18)
e2e/am-mock-api/src/app/routes.auth.js (1)
  • username (137-137)
e2e/journey-suites/src/utils/demo-user.ts (2)
  • username (7-7)
  • password (8-8)
⏰ 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 (17)
e2e/journey-app/package.json (1)

19-19: Workspace dependency addition is correct.

The @forgerock/protect dependency is properly added with the workspace:* protocol, consistent with other local package dependencies. This supports the Protect-related functionality (WebAuthn, recovery codes, etc.) referenced in the PR objectives.

e2e/journey-app/tsconfig.app.json (1)

22-24: Reference format is consistent with other library package references.

The new protect package reference follows the established pattern in tsconfig.app.json where library packages are referenced via their tsconfig.lib.json files, consistent with oidc-client and journey-client. The protect package's tsconfig.lib.json file exists at the referenced path.

e2e/journey-app/tsconfig.json (1)

23-25: Reference format is consistent with existing package references.

The new protect package reference follows the established pattern of other workspace package references (logger, oidc-client, journey-client) by pointing to the package directory without a tsconfig filename extension.

Verify that the protect package directory and its default tsconfig.json exist at packages/protect/.

e2e/am-mock-api/src/app/response.webauthn.js (1)

1-171: LGTM - Well-structured WebAuthn mock responses.

The module correctly encapsulates WebAuthn registration initialization and recovery codes display flow. The mock data structure follows the expected AM callback formats.

e2e/am-mock-api/src/app/responses.js (1)

1523-1603: LGTM - New Protect response exports.

protectInitializeResponse and protectEvaluateResponse are well-structured and follow the existing callback patterns for PingOne Protect flows.

e2e/am-mock-api/src/app/routes.auth.js (4)

410-410: Good defensive parsing.

Using JSON.parse(input.value || '{}') is a safer approach than parsing potentially undefined values directly. This prevents runtime errors when the input value is empty or missing.


346-367: LGTM - Protect journey flow logic.

The updated TEST_LoginPingProtect branch correctly handles the three-step flow: initialization → login → evaluation → success. The optional chaining on authId?.startsWith safely handles undefined authId.


434-452: LGTM - WebAuthn with recovery codes flow.

The WebAuthn flow correctly handles the multi-step process:

  1. MetadataCallback + HiddenValueCallback → returns recovery codes display
  2. ConfirmationCallback → sets session cookie and returns success

The conditional logic properly distinguishes between the different stages.


427-433: No action needed—the QR code condition is sufficiently specific.

The identifiers authIndexValue === 'QRCodeTest' and authId === 'qrcode-journey-confirmation' are used exclusively for QR code flows (see lines 110, 379, and 1409), and the AND conjunction with ConfirmationCallback further restricts the condition to specific QR code states. No other authentication flows use these identifiers.

e2e/journey-suites/src/qr-code.test.ts (1)

12-49: LGTM - Comprehensive QR code journey test.

The test covers the complete flow: login → QR code display → confirmation → completion → logout. The console message assertions verify the expected application behavior.

e2e/journey-app/components/ping-protect-evaluation.ts (1)

45-51: Good error handling for getData() response.

The code correctly checks if the result is an error object and handles it appropriately by setting the client error and updating the UI with visual feedback.

e2e/journey-suites/src/device-profile.test.ts (1)

12-47: LGTM! Well-structured e2e test.

The test properly validates the device profile collection flow with appropriate visibility checks, timeouts, and console message assertions.

e2e/journey-suites/src/recovery-codes.test.ts (1)

11-55: LGTM! Comprehensive test coverage for recovery codes flow.

The test properly validates the WebAuthn registration with recovery codes, including session token verification and logout flow.

e2e/journey-app/components/device-profile.ts (1)

14-26: LGTM! Clean component initialization.

The function properly sets up the message element and appends it to the journey container.

e2e/journey-suites/src/protect.test.ts (1)

24-50: LGTM! Comprehensive Protect flow validation.

The test properly validates the PingOne Protect initialization, risk assessment, and completion flow with appropriate visibility checks and console assertions.

e2e/journey-app/components/ping-protect-initialize.ts (2)

10-36: LGTM! Clean global instance management.

The global protectInstance storage with the getProtectInstance() getter provides a clean way for the evaluation component to access the initialized Protect SDK instance.


38-76: LGTM! Robust initialization with proper error handling.

The initialization flow properly validates configuration, handles errors gracefully, and provides clear user feedback through UI messages and callback error propagation.

nx-cloud[bot]

This comment was marked as outdated.

@ryanbas21 ryanbas21 force-pushed the journey-client-tests branch from 7edf5c0 to 2e8bca0 Compare December 3, 2025 16:54
Copy link
Contributor

@nx-cloud nx-cloud bot left a comment

Choose a reason for hiding this comment

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

Nx Cloud has identified a flaky task in your failed CI:

🔂 Since the failure was identified as flaky, we reran your CI automatically.

Nx Cloud View detailed reasoning in Nx Cloud ↗


🎓 Learn more about Self-Healing CI on nx.dev

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

♻️ Duplicate comments (1)
e2e/journey-suites/src/recovery-codes.test.ts (1)

55-56: Replace fixed timeout with a state-based wait after logout

Using await page.waitForTimeout(1000); makes this test slower and potentially flaky. Prefer waiting on a specific UI state that indicates the journey has reset (for example, the initial Submit button returning, or the URL/login prompt).

For example:

-    // Verify we're back at the beginning
-    await page.waitForTimeout(1000);
+    // Verify we're back at the beginning
+    await expect(page.getByRole('button', { name: 'Submit' })).toBeVisible({ timeout: 10000 });

This keeps the test deterministic and aligned with how the other waits in the suite are written.

🧹 Nitpick comments (3)
e2e/journey-app/components/device-profile.ts (1)

7-62: Device profile collection flow looks solid; only minor polish possible

The async collection using Device, the isLocationRequired / isMetadataRequired checks, and the auto-submit behavior all look correct and well-contained, with appropriate error handling and user feedback. If you ever need to tweak flakiness in tests, consider making the 100/500 ms setTimeout delays configurable or driven by DOM state rather than fixed times, but it’s not necessary for this e2e harness.

e2e/am-mock-api/src/app/responses.js (1)

1351-1435: qrCodeCallbacksResponse duplicates otpQRCodeCallbacks – consider sharing a common definition

qrCodeCallbacksResponse is effectively a copy of otpQRCodeCallbacks with a different authId. That’s fine for behavior, but it’s easy for one copy to drift if you ever tweak the script or hidden value.

You could factor the shared callback structure once and reuse it:

- export const otpQRCodeCallbacks = {
-  authId: 'foo',
-  callbacks: [
-    // ...
-  ],
-};
+const baseOtpQrCodeCallbacks = {
+  callbacks: [
+    // existing callbacks array here (TextOutput, HiddenValue, Confirmation)
+  ],
+};
+
+export const otpQRCodeCallbacks = {
+  authId: 'foo',
+  ...baseOtpQrCodeCallbacks,
+};
+
+export const qrCodeCallbacksResponse = {
+  authId: 'qrcode-journey-confirmation',
+  ...baseOtpQrCodeCallbacks,
+};

This keeps the QR-code script and callback wiring in one place.

e2e/am-mock-api/src/app/routes.auth.js (1)

400-420: Consider guarding JSON.parse and reusing callback lookups in DeviceProfile handling

The DeviceProfile branch now does:

const inputArr = deviceCb.input || [];
const input = inputArr[0] || {};
const value = JSON.parse(input.value || '{}');
const metadata = value.metadata || {};
// ...
const hasIdentifier = value.identifier && value.identifier.length > 0;

This is generally fine, but two optional robustness tweaks:

  • Wrap JSON.parse in a try/catch to avoid a 500 if input.value is non-JSON (e.g., a malformed payload from a future test), and fall back to {} on error.
  • Since you already call find for this callback once, you could store and reuse it to avoid multiple linear scans when this branch grows further.

Both are entirely optional in this mock/test API, but would make the handler a bit more defensive.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7edf5c0 and 2e8bca0.

📒 Files selected for processing (9)
  • e2e/am-mock-api/src/app/response.webauthn.js (1 hunks)
  • e2e/am-mock-api/src/app/responses.js (1 hunks)
  • e2e/am-mock-api/src/app/routes.auth.js (5 hunks)
  • e2e/journey-app/components/device-profile.ts (2 hunks)
  • e2e/journey-app/components/ping-protect-evaluation.ts (2 hunks)
  • e2e/journey-suites/src/device-profile.test.ts (1 hunks)
  • e2e/journey-suites/src/protect.test.ts (1 hunks)
  • e2e/journey-suites/src/qr-code.test.ts (1 hunks)
  • e2e/journey-suites/src/recovery-codes.test.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • e2e/journey-suites/src/device-profile.test.ts
  • e2e/journey-app/components/ping-protect-evaluation.ts
  • e2e/journey-suites/src/qr-code.test.ts
🧰 Additional context used
🧠 Learnings (2)
📚 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-suites/src/protect.test.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:

  • e2e/am-mock-api/src/app/responses.js
🧬 Code graph analysis (3)
e2e/journey-app/components/device-profile.ts (1)
packages/davinci-client/src/lib/node.slice.ts (1)
  • error (72-110)
e2e/am-mock-api/src/app/response.webauthn.js (1)
e2e/am-mock-api/src/app/responses.js (2)
  • authSuccess (45-49)
  • authSuccess (45-49)
e2e/journey-suites/src/protect.test.ts (2)
e2e/oidc-suites/src/utils/async-events.ts (2)
  • asyncEvents (7-76)
  • clickButton (9-18)
e2e/journey-suites/src/utils/demo-user.ts (2)
  • username (7-7)
  • password (8-8)
⏰ 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: Mend Code Security Check
  • GitHub Check: pr
🔇 Additional comments (3)
e2e/am-mock-api/src/app/response.webauthn.js (1)

15-171: WebAuthn + recovery-codes mocks align well with existing callback patterns

The webAuthnRegistrationInit metadata, hidden-value wiring, and getRecoveryCodesDisplay() flow (script TextOutput + confirmation) match the existing mock-callback patterns and what the new journey tests expect. The dedicated authSuccess object for WebAuthn keeps this flow nicely scoped. Given this is an e2e mock API, the inline script in the TextOutputCallback is appropriate here. Based on learnings, XSS hardening isn’t required in this test harness.

e2e/journey-suites/src/protect.test.ts (1)

12-49: Protect journey e2e flow looks consistent and robust

The test exercises initialization, login, risk evaluation, completion, and logout with clear UI assertions and console checks, and it aligns with the new TEST_LoginPingProtect routing. No issues spotted.

e2e/am-mock-api/src/app/routes.auth.js (1)

95-108: Journey routing for Protect, QR-code, DeviceProfile, and WebAuthn flows is well-structured

The new branches for:

  • TEST_LoginPingProtect + protect-journey-* authIds,
  • QR-code (QRCodeTest, qrcode-journey-login, qrcode-journey-confirmation),
  • device profile (DeviceProfileCallbackTest, device-profile-journey-*), and
  • WebAuthn with recovery codes (TEST_WebAuthnWithRecoveryCodes, webauthn-registration-*, recovery-codes-*)

are ordered correctly relative to the more generic PasswordCallback handling and appear to mirror the expectations in the new Playwright tests. Using distinct authId prefixes to track sub-states (init → login → eval/confirmation) keeps the flow easy to reason about, and the cookie setting for successful Protect/WebAuthn/QR-code paths matches existing patterns in this mock API.

Also applies to: 340-359, 368-375, 422-447

@ryanbas21 ryanbas21 force-pushed the journey-client-tests branch from d378c76 to ed1cb7f Compare December 3, 2025 20:56
@pkg-pr-new
Copy link

pkg-pr-new bot commented Dec 3, 2025

Open in StackBlitz

@forgerock/davinci-client

pnpm add https://pkg.pr.new/ForgeRock/ping-javascript-sdk/@forgerock/davinci-client@501

@forgerock/oidc-client

pnpm add https://pkg.pr.new/ForgeRock/ping-javascript-sdk/@forgerock/oidc-client@501

@forgerock/protect

pnpm add https://pkg.pr.new/ForgeRock/ping-javascript-sdk/@forgerock/protect@501

@forgerock/sdk-types

pnpm add https://pkg.pr.new/ForgeRock/ping-javascript-sdk/@forgerock/sdk-types@501

@forgerock/sdk-utilities

pnpm add https://pkg.pr.new/ForgeRock/ping-javascript-sdk/@forgerock/sdk-utilities@501

@forgerock/iframe-manager

pnpm add https://pkg.pr.new/ForgeRock/ping-javascript-sdk/@forgerock/iframe-manager@501

@forgerock/sdk-logger

pnpm add https://pkg.pr.new/ForgeRock/ping-javascript-sdk/@forgerock/sdk-logger@501

@forgerock/sdk-oidc

pnpm add https://pkg.pr.new/ForgeRock/ping-javascript-sdk/@forgerock/sdk-oidc@501

@forgerock/sdk-request-middleware

pnpm add https://pkg.pr.new/ForgeRock/ping-javascript-sdk/@forgerock/sdk-request-middleware@501

@forgerock/storage

pnpm add https://pkg.pr.new/ForgeRock/ping-javascript-sdk/@forgerock/storage@501

commit: 00143a5

@codecov-commenter
Copy link

codecov-commenter commented Dec 3, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 18.81%. Comparing base (b89ad58) to head (00143a5).
⚠️ Report is 9 commits behind head on main.

❌ Your project status has failed because the head coverage (18.81%) is below the target coverage (40.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #501      +/-   ##
==========================================
+ Coverage   18.79%   18.81%   +0.02%     
==========================================
  Files         140      140              
  Lines       27640    27647       +7     
  Branches      980      981       +1     
==========================================
+ Hits         5195     5202       +7     
  Misses      22445    22445              

see 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 3, 2025

Deployed f87a564 to https://ForgeRock.github.io/ping-javascript-sdk/pr-501/f87a564a5a77785d991f3258582ff225fc74f3a5 branch gh-pages in ForgeRock/ping-javascript-sdk

@github-actions
Copy link
Contributor

github-actions bot commented Dec 3, 2025

📦 Bundle Size Analysis

📦 Bundle Size Analysis

🚨 Significant Changes

🔻 @forgerock/journey-client - 0.0 KB (-82.4 KB, -100.0%)

📊 Minor Changes

📈 @forgerock/journey-client - 82.4 KB (+0.1 KB)

➖ No Changes

@forgerock/device-client - 9.2 KB
@forgerock/oidc-client - 23.4 KB
@forgerock/protect - 150.1 KB
@forgerock/sdk-utilities - 7.5 KB
@forgerock/sdk-types - 8.0 KB
@forgerock/storage - 1.4 KB
@forgerock/sdk-logger - 1.6 KB
@forgerock/iframe-manager - 2.4 KB
@forgerock/sdk-request-middleware - 4.5 KB
@forgerock/sdk-oidc - 2.6 KB
@forgerock/davinci-client - 39.5 KB


13 packages analyzed • Baseline from latest main build

Legend

🆕 New package
🔺 Size increased
🔻 Size decreased
➖ No change

ℹ️ How bundle sizes are calculated
  • Current Size: Total gzipped size of all files in the package's dist directory
  • Baseline: Comparison against the latest build from the main branch
  • Files included: All build outputs except source maps and TypeScript build cache
  • Exclusions: .map, .tsbuildinfo, and .d.ts.map files

🔄 Updated automatically on each push to this PR

Copy link
Collaborator

@cerebrl cerebrl left a comment

Choose a reason for hiding this comment

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

Let's wait for this PR to have a bit better direct, explicit testing, rather than indirect testing for these additional modules.

Comment on lines 51 to 54
const submitButton = document.getElementById('submitButton') as HTMLButtonElement;
if (submitButton) {
submitButton.click();
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we not have a way to call the submit function without having to create a button and auto-clicking it?

await clickButton('Submit', '/authenticate');

// Step 2: QR Code step should be displayed with instruction message
await expect(page.getByText('Scan the QR code image below', { exact: false })).toBeVisible({
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this is testing the dedicated QR Code module since it isn't used in the test app. I don't think testing the message string itself is sufficient.

// Step 2: Recovery Codes display step
// Should show the TextOutputCallback with recovery codes
// and a ConfirmationCallback with radio button option
await expect(page.getByText('I have saved my recovery codes')).toBeVisible({ timeout: 10000 });
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as the QR Code test; I don't believe this is testing the dedicated Recovery Codes module. So, this should be reworked.

Comment on lines +29 to +34
await expect(page.getByText('Collecting device profile information...')).toBeVisible({
timeout: 10000,
});
await expect(page.getByText('Device profile collected successfully!')).toBeVisible({
timeout: 15000,
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't mind testing for these messages, indirectly testing Device Profile, but I think we need to more directly test the feature by grabbing the request body, parsing it and ensuring the JSON at least has the expected base properties with truthy values.

Comment on lines +34 to +37
await expect(page.getByText('Evaluating risk assessment...')).toBeVisible({ timeout: 10000 });
await expect(page.getByText('Risk assessment completed successfully!')).toBeVisible({
timeout: 15000,
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as the Device Profile test. Let's test the request body directly, rather than just testing for messages.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (3)
e2e/journey-app/components/recovery-codes.ts (1)

10-114: RecoveryCodes rendering looks correct; consider toning down sensitive debug logs

The step detection and rendering (codes grid, optional device, and confirmation radio wiring) look consistent with the RecoveryCodes API and should behave well in the journey app.

One minor concern: the debug logs currently print the full recovery codes and device name:

  • Lines 18–20 log all codes and the device name to the console.
  • These values are also indirectly asserted in tests via the log messages.

Given this lives in the e2e app against mock data, it’s acceptable, but if this component were ever reused outside the test harness it would be safer to log only non-sensitive metadata (e.g., code count) or gate these logs behind a debug flag.

e2e/journey-app/components/qr-code.ts (1)

10-87: QR Code step rendering is solid; be cautious with logging full QR data

The QR step handler correctly:

  • Detects QR steps,
  • Renders message/URI/use-type elements with stable IDs used by the tests, and
  • Wires the ConfirmationCallback radios including the default option.

One small nit: lines 17–19 and 28–41 log the full QR payload and URI, which typically contains a secret. That’s fine for the mock e2e app, but if this component is ever reused beyond tests, consider switching to less-sensitive logging (e.g., logging only the use-type and a redacted URI) or gating these logs behind an explicit debug flag.

e2e/journey-app/components/ping-protect-evaluation.ts (1)

27-72: Protect evaluation flow looks good; consider tightening the error-type guard

The async collection/auto-submit behavior and UI messaging look consistent with the journey tests and give clear feedback for both success and failure. One small robustness tweak: the 'error' in result check can currently be reached even if result is null or another non-object value, relying on the outer try/catch to recover.

To make the intent clearer and avoid relying on the catch block for such shape mismatches, you could explicitly guard for an object before checking for error:

-      // Collect device and behavioral data
-      const result = await protectInstance.getData();
-
-      // Check if result is an error object
-      if (typeof result !== 'string' && 'error' in result) {
+      // Collect device and behavioral data
+      const result = await protectInstance.getData();
+
+      // Check if result is an error object
+      if (result && typeof result === 'object' && 'error' in result) {
         console.error('Error collecting Protect data:', result.error);
         callback.setClientError(result.error);
         message.innerText = `Data collection failed: ${result.error}`;
         message.style.color = 'red';
         return;
       }

This keeps the behavior the same for the expected return types while making the error-path handling more explicit and future-proof if getData() ever returns other falsy values.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ed1cb7f and b1a1a23.

📒 Files selected for processing (10)
  • e2e/journey-app/components/device-profile.ts (2 hunks)
  • e2e/journey-app/components/ping-protect-evaluation.ts (2 hunks)
  • e2e/journey-app/components/ping-protect-initialize.ts (2 hunks)
  • e2e/journey-app/components/qr-code.ts (1 hunks)
  • e2e/journey-app/components/recovery-codes.ts (1 hunks)
  • e2e/journey-app/main.ts (2 hunks)
  • e2e/journey-suites/src/device-profile.test.ts (1 hunks)
  • e2e/journey-suites/src/protect.test.ts (1 hunks)
  • e2e/journey-suites/src/qr-code.test.ts (1 hunks)
  • e2e/journey-suites/src/recovery-codes.test.ts (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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:

  • e2e/journey-app/main.ts
🧬 Code graph analysis (8)
e2e/journey-app/components/recovery-codes.ts (1)
packages/journey-client/src/lib/callbacks/confirmation-callback.ts (1)
  • ConfirmationCallback (14-78)
e2e/journey-app/main.ts (3)
e2e/journey-app/components/qr-code.ts (1)
  • renderQRCodeStep (10-88)
e2e/journey-app/components/recovery-codes.ts (1)
  • renderRecoveryCodesStep (10-115)
e2e/journey-app/callback-map.ts (1)
  • renderCallbacks (153-157)
e2e/journey-app/components/ping-protect-evaluation.ts (2)
e2e/journey-app/components/ping-protect-initialize.ts (1)
  • getProtectInstance (17-19)
packages/davinci-client/src/lib/node.slice.ts (1)
  • error (72-110)
e2e/journey-app/components/device-profile.ts (1)
packages/journey-client/src/lib/callbacks/device-profile-callback.ts (2)
  • isLocationRequired (41-43)
  • isMetadataRequired (34-36)
e2e/journey-suites/src/qr-code.test.ts (1)
e2e/am-mock-api/src/app/routes.auth.js (1)
  • username (131-131)
e2e/journey-app/components/qr-code.ts (1)
packages/journey-client/src/lib/callbacks/confirmation-callback.ts (1)
  • ConfirmationCallback (14-78)
e2e/journey-suites/src/protect.test.ts (2)
e2e/oidc-suites/src/utils/async-events.ts (2)
  • asyncEvents (7-76)
  • clickButton (9-18)
e2e/journey-suites/src/utils/demo-user.ts (2)
  • username (7-7)
  • password (8-8)
e2e/journey-suites/src/device-profile.test.ts (2)
e2e/oidc-suites/src/utils/async-events.ts (2)
  • navigate (54-56)
  • clickButton (9-18)
e2e/journey-suites/src/utils/demo-user.ts (2)
  • username (7-7)
  • password (8-8)
⏰ 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: Mend Code Security Check
  • GitHub Check: pr
🔇 Additional comments (7)
e2e/journey-app/main.ts (1)

14-15: Specialized QR/RecoveryCodes rendering integration looks correct

Importing the QR and RecoveryCodes renderers and using:

const stepRendered =
  renderQRCodeStep(journeyEl, step) || renderRecoveryCodesStep(journeyEl, step);

if (!stepRendered) {
  renderCallbacks(journeyEl, step.callbacks);
}

provides a clean, non-invasive way to prefer specialized step UIs while preserving the existing callback-based rendering for all other steps. The behavior for non-QR/non-recovery steps remains unchanged, and the submit button is still rendered once per form.

Also applies to: 120-126

e2e/journey-suites/src/qr-code.test.ts (1)

12-56: QR Code e2e test meaningfully exercises the dedicated QRCode module

This test does a good job of:

  • Driving the QRCodeTest journey end-to-end (login → QR step → completion → logout),
  • Verifying the rendered QR UI (message, otpauth URI, Type: otp), and
  • Asserting console output that confirms the QRCode module was used and that the journey completed and logged out successfully.

The expectations line up well with the new renderer and main.ts wiring.

e2e/journey-suites/src/recovery-codes.test.ts (1)

11-65: Recovery Codes e2e test now directly validates the RecoveryCodes module

This test:

  • Navigates the WebAuthn + Recovery Codes journey, submits the registration step, and verifies the recovery-codes UI (container, header text, codes list, at least one non-empty code).
  • Confirms the presence of the “I have saved my recovery codes” prompt and successful completion/logout.
  • Uses console assertions to confirm the RecoveryCodes module was invoked and the journey completed.

Switching to explicit visibility checks after logout (instead of a fixed timeout) is a nice robustness improvement.

e2e/journey-app/components/device-profile.ts (1)

7-63: Device profile collection component aligns well with callback semantics and tests

The updated component cleanly:

  • Derives required flags via isLocationRequired() / isMetadataRequired(),
  • Uses the Device helper to collect a profile and sets it on the callback,
  • Surfaces status to the user via clear success/error messages, and
  • Auto-submits the form after successful collection using form.requestSubmit(), which ties into the existing submit handler without extra wiring.

This fits the journey model and pairs nicely with the new device-profile e2e test that inspects the posted profile payload.

e2e/journey-app/components/ping-protect-initialize.ts (1)

7-91: Protect initialization component provides clear lifecycle and error handling

The new implementation:

  • Maintains a single shared Protect instance at module scope with a simple getter for the evaluation component,
  • Reads configuration from the callback and explicitly guards against missing envId,
  • Handles both start-time errors (via result?.error) and thrown exceptions by setting setClientError and updating the UI, and
  • Auto-submits the form after successful initialization so the journey can proceed without manual interaction.

This structure should make the Protect flows in the e2e journeys predictable and debuggable.

e2e/journey-suites/src/device-profile.test.ts (1)

12-90: Device profile e2e test now robustly validates the collected profile payload

This test significantly improves coverage by:

  • Intercepting POST /authenticate requests and extracting the JSON-encoded device profile from callback inputs,
  • Asserting both UI transitions (“Collecting…”, “Device profile collected successfully!”, “Complete”) and the shape of the posted profile (identifier, metadata.hardware/browser/platform.deviceName), and
  • Verifying journey lifecycle through console logs and a full logout/back-to-login cycle.

The defensive parsing and null checks make the interception logic resilient while giving strong guarantees that the DeviceProfileCallback is populated as expected.

e2e/journey-suites/src/protect.test.ts (1)

12-83: E2E Protect journey coverage is strong and addresses prior payload-check feedback

This test now exercises the full Protect journey end-to-end: it asserts the init/evaluation UI states, waits for the auto-submitted evaluation, and verifies completion plus logout. The /authenticate request listener that drills into PingOneProtectEvaluationCallback and checks IDToken1signals gives a solid, payload-level assertion rather than just relying on messages, and the console-log checks further validate the Protect SDK flow.

The structure and timeouts look reasonable for avoiding flakes while keeping the test readable; I don’t see functional issues here.

@ryanbas21 ryanbas21 force-pushed the journey-client-tests branch from b1a1a23 to ac7fe33 Compare December 4, 2025 16:45
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (4)
e2e/journey-app/components/qr-code.ts (1)

68-75: Redundant setOptionIndex call for default option.

The default option index is set both in the change event handler (when radio is already checked on mount) and explicitly again on lines 73-75. Since the checked state is set on line 68, the explicit call in the condition block is redundant.

       radio.addEventListener('change', () => {
         confirmCb.setOptionIndex(index);
       });

-      if (index === confirmCb.getDefaultOption()) {
-        confirmCb.setOptionIndex(index);
-      }
+      // Initialize the callback with the default option value
+      if (radio.checked) {
+        confirmCb.setOptionIndex(index);
+      }

Alternatively, initialize the default once before the loop:

confirmCb.setOptionIndex(confirmCb.getDefaultOption());
e2e/journey-suites/src/qr-code.test.ts (1)

16-19: Consider removing the unnecessary return statement in console handler.

The Promise.resolve(true) return is unnecessary since the console event handler doesn't require a return value.

   page.on('console', async (msg) => {
     messageArray.push(msg.text());
-    return Promise.resolve(true);
   });
e2e/journey-suites/src/recovery-codes.test.ts (1)

18-21: Same cleanup opportunity as QR code test.

The Promise.resolve(true) return is unnecessary in the console handler.

     page.on('console', async (msg) => {
       messageArray.push(msg.text());
-      return Promise.resolve(true);
     });
e2e/journey-suites/src/device-profile.test.ts (1)

17-20: Same console handler cleanup opportunity.

   page.on('console', async (msg) => {
     messageArray.push(msg.text());
-    return Promise.resolve(true);
   });
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b1a1a23 and ac7fe33.

📒 Files selected for processing (10)
  • e2e/journey-app/components/device-profile.ts (2 hunks)
  • e2e/journey-app/components/ping-protect-evaluation.ts (2 hunks)
  • e2e/journey-app/components/ping-protect-initialize.ts (2 hunks)
  • e2e/journey-app/components/qr-code.ts (1 hunks)
  • e2e/journey-app/components/recovery-codes.ts (1 hunks)
  • e2e/journey-app/main.ts (2 hunks)
  • e2e/journey-suites/src/device-profile.test.ts (1 hunks)
  • e2e/journey-suites/src/protect.test.ts (1 hunks)
  • e2e/journey-suites/src/qr-code.test.ts (1 hunks)
  • e2e/journey-suites/src/recovery-codes.test.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • e2e/journey-app/components/recovery-codes.ts
  • e2e/journey-app/components/ping-protect-initialize.ts
  • e2e/journey-suites/src/protect.test.ts
🧰 Additional context used
🧠 Learnings (1)
📚 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:

  • e2e/journey-app/main.ts
🧬 Code graph analysis (5)
e2e/journey-app/components/qr-code.ts (1)
packages/journey-client/src/lib/callbacks/confirmation-callback.ts (1)
  • ConfirmationCallback (14-78)
e2e/journey-app/components/ping-protect-evaluation.ts (1)
e2e/journey-app/components/ping-protect-initialize.ts (1)
  • getProtectInstance (17-19)
e2e/journey-app/main.ts (3)
e2e/journey-app/components/qr-code.ts (1)
  • renderQRCodeStep (10-88)
e2e/journey-app/components/recovery-codes.ts (1)
  • renderRecoveryCodesStep (10-115)
e2e/journey-app/callback-map.ts (1)
  • renderCallbacks (153-157)
e2e/journey-suites/src/qr-code.test.ts (1)
e2e/am-mock-api/src/app/routes.auth.js (1)
  • username (131-131)
e2e/journey-suites/src/device-profile.test.ts (1)
e2e/journey-suites/src/utils/demo-user.ts (2)
  • username (7-7)
  • password (8-8)
⏰ 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). (3)
  • GitHub Check: Mend Code Security Check
  • GitHub Check: Mend Security Check
  • GitHub Check: pr
🔇 Additional comments (8)
e2e/journey-app/components/ping-protect-evaluation.ts (1)

27-73: Solid async data collection implementation with comprehensive error handling.

The implementation properly handles both error objects returned from getData() and exceptions thrown during execution. The use of form.requestSubmit() is the correct modern approach for programmatic form submission.

One minor observation: the nested setTimeout pattern (outer 100ms, inner 500ms for auto-submit) works but could potentially be simplified if the SDK provided a callback-based flow. However, this is acceptable for an e2e test app.

e2e/journey-app/components/qr-code.ts (1)

10-88: Well-structured step renderer with proper guard pattern.

The implementation correctly uses the QRCode.isQRCodeStep guard to detect and handle QR code steps, returning false early for non-matching steps. The radio button wiring for ConfirmationCallback properly uses setOptionIndex per the relevant code snippet from confirmation-callback.ts.

e2e/journey-suites/src/qr-code.test.ts (1)

12-56: Good test coverage verifying QR Code module integration.

The test effectively validates the QR Code module is being used by:

  1. Verifying the rendered UI elements (#qr-code-container, #qr-code-uri, etc.)
  2. Asserting console messages include "QR Code step detected via QRCode module" (lines 50-52), which directly proves the QRCode module from @forgerock/journey-client/qr-code is invoked

This addresses the earlier concern about whether the dedicated QR Code module is actually being tested.

e2e/journey-app/main.ts (1)

120-126: Clean step-type prioritization pattern.

The early-out approach with || short-circuit evaluation correctly tries specialized renderers before falling back to renderCallbacks. This pattern is extensible for future step types.

Note: If a step could theoretically contain both QR code and recovery codes elements, only the QR code renderer would execute due to short-circuit. Based on the journey flow design, this appears to be the intended behavior.

e2e/journey-suites/src/recovery-codes.test.ts (1)

11-65: Comprehensive test validating RecoveryCodes module integration.

The test properly validates:

  1. UI elements rendered by the RecoveryCodes module (#recovery-codes-container, #recovery-codes-header, .recovery-code)
  2. Console message assertion (lines 57-61) confirms "Recovery Codes step detected via RecoveryCodes module" is logged, proving the dedicated module is invoked

This addresses the earlier concern about directly testing the Recovery Codes module.

e2e/journey-app/components/device-profile.ts (1)

27-62: Proper Device class integration with correct form submission approach.

The implementation correctly:

  1. Uses Device class from the journey-client SDK
  2. Respects callback requirements via isLocationRequired() and isMetadataRequired()
  3. Uses form.requestSubmit() for programmatic submission (not button clicking)
  4. Provides comprehensive error handling with user feedback

The pattern is consistent with ping-protect-evaluation.ts, which aids maintainability.

e2e/journey-suites/src/device-profile.test.ts (2)

22-44: Good implementation of request interception to validate device profile data.

This directly addresses the earlier feedback to "more directly test the feature by grabbing the request body." The interception correctly:

  1. Filters for /authenticate POST requests
  2. Parses the DeviceProfileCallback from callbacks array
  3. Extracts the nested JSON from IDToken1 input value

62-75: Thorough validation of device profile structure.

The assertions properly verify the captured device profile contains expected properties:

  • identifier (string, non-empty)
  • metadata.hardware, metadata.browser, metadata.platform
  • platform.deviceName

This provides confidence that the Device class is correctly collecting and serializing profile data.

Copy link
Collaborator

@cerebrl cerebrl left a comment

Choose a reason for hiding this comment

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

Though I'd prefer to pass a submit form function from parent to child in the test application, I wouldn't call it blocking as what you did works. I just prefer the child component not to have to "know" anything outside of itself.

Comment on lines 51 to 54
const form = document.getElementById('form') as HTMLFormElement;
if (form) {
form.requestSubmit();
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd much rather have a function that the parent passes to the child to submit the form without having to select the actual DOM element and calling a function off of that.

@ryanbas21 ryanbas21 merged commit cb7b20c into main Dec 10, 2025
7 checks passed
@ryanbas21 ryanbas21 deleted the journey-client-tests branch December 10, 2025 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants