Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion .github/actions/setup/action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,9 @@ runs:
registry-url: 'https://registry.npmjs.org'

- name: Update npm
run: npm install -g npm@latest
run: |
corepack enable
corepack install -g npm@latest
Comment on lines +34 to +36
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify all floating npm installs in CI/config are removed
rg -n --iglob '.github/**' 'npm@latest|npm install -g npm@latest|corepack install -g npm@latest'

Repository: ForgeRock/ping-javascript-sdk

Length of output: 221


🏁 Script executed:

# First, examine the specific file mentioned in the review
cat -n .github/actions/setup/action.yml | sed -n '30,40p'

Repository: ForgeRock/ping-javascript-sdk

Length of output: 416


🏁 Script executed:

# Fix the ripgrep search - the previous glob pattern had issues
# Search for npm@latest in .github directory with corrected syntax
rg 'npm@latest' .github/ -n

Repository: ForgeRock/ping-javascript-sdk

Length of output: 219


🏁 Script executed:

# Also search for corepack install patterns to get full context
rg 'corepack install' .github/ -n -A 1 -B 1

Repository: ForgeRock/ping-javascript-sdk

Length of output: 261


Pin npm versions in CI instead of using npm@latest.

Found two locations installing npm without a pinned version:

  • .github/actions/setup/action.yml:36corepack install -g npm@latest
  • .github/workflows/publish.yml:54npm install npm@latest -g

Using npm@latest makes CI non-reproducible and can break unexpectedly when npm releases. Pin to a specific version for deterministic builds.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/actions/setup/action.yml around lines 34 - 36, Replace the unpinned
npm installs by pinning to a specific version (or a workflow input variable)
instead of using "npm@latest": change the two occurrences of the command strings
"corepack install -g npm@latest" and "npm install npm@latest -g" to use a
concrete version like "npm@<X.Y.Z>" or "${{ inputs.npm_version }}"/environment
variable; ensure the chosen version is defined centrally (workflow/input/default
or repo-level constant) so both places use the same pinned value and update any
documentation or CI inputs accordingly.

shell: bash

- name: Install dependencies
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ jobs:
with:
CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }}

- run: npx nx-cloud fix-ci
- run: pnpm nx fix-ci
if: always()

- uses: codecov/codecov-action@v5
Expand Down
58 changes: 58 additions & 0 deletions e2e/davinci-app/components/polling.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
/*
* Copyright (c) 2025 Ping Identity Corporation. All rights reserved.
*
* This software may be modified and distributed under the terms
* of the MIT license. See the LICENSE file for details.
*/
import type {
PollingCollector,
PollingStatus,
InternalErrorResponse,
Updater,
} from '@forgerock/davinci-client/types';

export default function pollingComponent(
formEl: HTMLFormElement,
collector: PollingCollector,
poll: (collector: PollingCollector) => Promise<PollingStatus | InternalErrorResponse>,
updater: Updater<PollingCollector>,
submitForm: () => Promise<void>,
) {
const button = document.createElement('button');
button.type = 'button';
button.value = collector.output.key;
button.innerHTML = 'Start polling';
formEl.appendChild(button);

button.onclick = async () => {
const p = document.createElement('p');
p.innerText = 'Polling...';
formEl?.appendChild(p);

const status = await poll(collector);
if (typeof status !== 'string' && 'error' in status) {
console.error(status.error?.message);

const errEl = document.createElement('p');
errEl.innerText = 'Polling error: ' + status.error?.message;
formEl?.appendChild(errEl);
return;
}

const result = updater(status);
if (result && 'error' in result) {
console.error(result.error.message);

const errEl = document.createElement('p');
errEl.innerText = 'Polling error: ' + result.error.message;
formEl?.appendChild(errEl);
return;
}

const resultEl = document.createElement('p');
resultEl.innerText = 'Polling result: ' + JSON.stringify(status, null, 2);
formEl?.appendChild(resultEl);

await submitForm();
};
Comment on lines +27 to +57
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Block re-entry while a poll is in flight.

The button stays clickable during both poll() and submitForm(), so a double-click can start overlapping poll/update/submit sequences for the same interaction. Disable it for the whole async section and restore it in finally.

💡 Proposed fix
   button.onclick = async () => {
-    const p = document.createElement('p');
-    p.innerText = 'Polling...';
-    formEl?.appendChild(p);
-
-    const status = await poll(collector);
-    if (typeof status !== 'string' && 'error' in status) {
-      console.error(status.error?.message);
-
-      const errEl = document.createElement('p');
-      errEl.innerText = 'Polling error: ' + status.error?.message;
-      formEl?.appendChild(errEl);
-      return;
-    }
-
-    const result = updater(status);
-    if (result && 'error' in result) {
-      console.error(result.error.message);
-
-      const errEl = document.createElement('p');
-      errEl.innerText = 'Polling error: ' + result.error.message;
-      formEl?.appendChild(errEl);
-      return;
-    }
-
-    const resultEl = document.createElement('p');
-    resultEl.innerText = 'Polling result: ' + JSON.stringify(status, null, 2);
-    formEl?.appendChild(resultEl);
-
-    await submitForm();
+    button.disabled = true;
+    try {
+      const p = document.createElement('p');
+      p.innerText = 'Polling...';
+      formEl?.appendChild(p);
+
+      const status = await poll(collector);
+      if (typeof status !== 'string' && 'error' in status) {
+        console.error(status.error?.message);
+
+        const errEl = document.createElement('p');
+        errEl.innerText = 'Polling error: ' + status.error?.message;
+        formEl?.appendChild(errEl);
+        return;
+      }
+
+      const result = updater(status);
+      if (result && 'error' in result) {
+        console.error(result.error.message);
+
+        const errEl = document.createElement('p');
+        errEl.innerText = 'Polling error: ' + result.error.message;
+        formEl?.appendChild(errEl);
+        return;
+      }
+
+      const resultEl = document.createElement('p');
+      resultEl.innerText = 'Polling result: ' + JSON.stringify(status, null, 2);
+      formEl?.appendChild(resultEl);
+
+      await submitForm();
+    } finally {
+      button.disabled = false;
+    }
   };
📝 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.

Suggested change
button.onclick = async () => {
const p = document.createElement('p');
p.innerText = 'Polling...';
formEl?.appendChild(p);
const status = await poll(collector);
if (typeof status !== 'string' && 'error' in status) {
console.error(status.error?.message);
const errEl = document.createElement('p');
errEl.innerText = 'Polling error: ' + status.error?.message;
formEl?.appendChild(errEl);
return;
}
const result = updater(status);
if (result && 'error' in result) {
console.error(result.error.message);
const errEl = document.createElement('p');
errEl.innerText = 'Polling error: ' + result.error.message;
formEl?.appendChild(errEl);
return;
}
const resultEl = document.createElement('p');
resultEl.innerText = 'Polling result: ' + JSON.stringify(status, null, 2);
formEl?.appendChild(resultEl);
await submitForm();
};
button.onclick = async () => {
button.disabled = true;
try {
const p = document.createElement('p');
p.innerText = 'Polling...';
formEl?.appendChild(p);
const status = await poll(collector);
if (typeof status !== 'string' && 'error' in status) {
console.error(status.error?.message);
const errEl = document.createElement('p');
errEl.innerText = 'Polling error: ' + status.error?.message;
formEl?.appendChild(errEl);
return;
}
const result = updater(status);
if (result && 'error' in result) {
console.error(result.error.message);
const errEl = document.createElement('p');
errEl.innerText = 'Polling error: ' + result.error.message;
formEl?.appendChild(errEl);
return;
}
const resultEl = document.createElement('p');
resultEl.innerText = 'Polling result: ' + JSON.stringify(status, null, 2);
formEl?.appendChild(resultEl);
await submitForm();
} finally {
button.disabled = false;
}
};
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@e2e/davinci-app/components/polling.ts` around lines 27 - 57, The click
handler (button.onclick) allows re-entry while async work runs; disable the
button at the start of the async sequence and re-enable it in a finally block so
overlapping poll()/updater()/submitForm() runs cannot start; wrap the existing
body of button.onclick in try { ... } finally { button.disabled = false } (set
button.disabled = true before calling poll) and ensure all early returns still
lead to the finally path so the button is always restored.

}
9 changes: 9 additions & 0 deletions e2e/davinci-app/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import multiValueComponent from './components/multi-value.js';
import labelComponent from './components/label.js';
import objectValueComponent from './components/object-value.js';
import fidoComponent from './components/fido.js';
import pollingComponent from './components/polling.js';

const loggerFn = {
error: () => {
Expand Down Expand Up @@ -262,6 +263,14 @@ const urlParams = new URLSearchParams(window.location.search);
davinciClient.update(collector), // Returns an update function for this collector
submitForm,
);
} else if (collector.type === 'PollingCollector') {
pollingComponent(
formEl, // You can ignore this; it's just for rendering
collector, // This is the plain object of the collector
davinciClient.poll, // Returns a poll function
davinciClient.update(collector), // Returns an update function for this collector
submitForm,
);
} else if (collector.type === 'FlowCollector') {
flowLinkComponent(
formEl, // You can ignore this; it's just for rendering
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
},
"author": "ForgeRock",
"scripts": {
"build": "nx affected --target=build",
"build": "nx sync && nx affected --target=build",
"changeset": "changeset",
"ci:release": "pnpm nx run-many -t build --no-agents && pnpm publish -r --no-git-checks && changeset tag",
"ci:version": "changeset version && pnpm install --no-frozen-lockfile && pnpm nx format:write --uncommitted",
Expand Down
95 changes: 91 additions & 4 deletions packages/davinci-client/src/lib/client.store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,20 @@
* This software may be modified and distributed under the terms
* of the MIT license. See the LICENSE file for details.
*/
/**
* Import RTK slices and api
*/
import { Micro } from 'effect';
import { CustomLogger, logger as loggerFn, LogLevel } from '@forgerock/sdk-logger';
import { createStorage } from '@forgerock/storage';
import { isGenericError, createWellknownError } from '@forgerock/sdk-utilities';

import { createClientStore, handleUpdateValidateError, RootState } from './client.store.utils.js';
/**
* Import RTK slices and api
*/
import {
createClientStore,
handleChallengePolling,
handleUpdateValidateError,
RootState,
} from './client.store.utils.js';
import { nodeSlice } from './node.slice.js';
import { davinciApi } from './davinci.api.js';
import { configSlice } from './config.slice.js';
Expand All @@ -34,6 +40,7 @@ import type {
ObjectValueCollectors,
PhoneNumberInputValue,
AutoCollectors,
PollingCollector,
MultiValueCollectors,
FidoRegistrationInputValue,
FidoAuthenticationInputValue,
Expand All @@ -44,6 +51,7 @@ import type {
NodeStates,
Updater,
Validator,
PollingStatus,
} from './client.types.js';
import { returnValidator } from './collector.utils.js';
import type { ContinueNode, StartNode } from './node.types.js';
Expand Down Expand Up @@ -404,6 +412,85 @@ export async function davinci<ActionType extends ActionTypes = ActionTypes>({
return returnValidator(collectorToUpdate);
},

/**
* @method poll - Perform challenge polling or continue polling
* @param {PollingCollector} collector - the polling collector
* @returns {Promise<PollingStatus | InternalErrorResponse>} - Returns a promise that resolves to a polling status or error
*/
poll: async (collector: PollingCollector): Promise<PollingStatus | InternalErrorResponse> => {
try {
if (collector.type !== 'PollingCollector') {
log.error('Collector provided to poll is not a PollingCollector');
return {
error: {
message: 'Collector provided to poll is not a PollingCollector',
type: 'argument_error',
},
type: 'internal_error',
};
}

const pollChallengeStatus = collector.output.config.pollChallengeStatus;
const challenge = collector.output.config.challenge;

if (challenge && pollChallengeStatus === true) {
// Challenge Polling
return await handleChallengePolling({
collector,
challenge,
store,
log,
});
} else if (!challenge && !pollChallengeStatus) {
// Continue polling
const retriesLeft = collector.output.config.retriesRemaining;
const pollInterval = collector.output.config.pollInterval ?? 2000; // miliseconds

if (retriesLeft === undefined) {
log.error('No retries found on PollingCollector');
return {
error: {
message: 'No retries found on PollingCollector',
type: 'argument_error',
},
type: 'internal_error',
};
}

if (retriesLeft > 0) {
const getStatusµ = Micro.sync(() => 'continue' as PollingStatus).pipe(
Micro.delay(pollInterval),
);
const status: PollingStatus = await Micro.runPromise(getStatusµ);
return status;
} else {
// Retries exhausted
return 'timedOut' as PollingStatus;
}
} else {
// Error if polling type can't be determined from configuration
log.error('Invalid polling collector configuration');
return {
error: {
message: 'Invalid polling collector configuration',
type: 'internal_error',
},
type: 'internal_error',
};
}
Comment on lines +420 to +480
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Resolve the polling collector from store before using its config.

Unlike update() and validate(), this method trusts the caller's collector snapshot. After a node.poll state update, external references are stale, so repeated poll(collector) calls can reuse old retriesRemaining / pollInterval values or keep polling after the node changed. Re-select by collector.id on entry and use the current store-backed collector from there.

💡 Proposed fix
     poll: async (collector: PollingCollector): Promise<PollingStatus | InternalErrorResponse> => {
       try {
         if (collector.type !== 'PollingCollector') {
           log.error('Collector provided to poll is not a PollingCollector');
           return {
@@
             type: 'internal_error',
           };
         }
 
-        const pollChallengeStatus = collector.output.config.pollChallengeStatus;
-        const challenge = collector.output.config.challenge;
+        const { error, state: currentCollector } = nodeSlice.selectors.selectCollector(
+          store.getState(),
+          collector.id,
+        );
+
+        if (error || !currentCollector || currentCollector.type !== 'PollingCollector') {
+          log.error('PollingCollector not found in current state');
+          return {
+            error: {
+              message: 'PollingCollector not found in current state',
+              type: 'state_error',
+            },
+            type: 'internal_error',
+          };
+        }
+
+        const pollChallengeStatus = currentCollector.output.config.pollChallengeStatus;
+        const challenge = currentCollector.output.config.challenge;
 
         if (challenge && pollChallengeStatus === true) {
           // Challenge Polling
           return await handleChallengePolling({
-            collector,
+            collector: currentCollector,
             challenge,
             store,
             log,
           });
         } else if (!challenge && !pollChallengeStatus) {
           // Continue polling
-          const retriesLeft = collector.output.config.retriesRemaining;
-          const pollInterval = collector.output.config.pollInterval ?? 2000; // miliseconds
+          const retriesLeft = currentCollector.output.config.retriesRemaining;
+          const pollInterval = currentCollector.output.config.pollInterval ?? 2000; // miliseconds
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/davinci-client/src/lib/client.store.ts` around lines 420 - 480, The
poll implementation in poll(collector: PollingCollector) trusts the caller's
collector snapshot and can operate on stale config; on entry re-resolve the
collector from the store by collector.id (e.g. fetch the current collector
object from store using the passed collector.id) and use that store-backed
collector for reading output.config (retriesRemaining, pollInterval, challenge,
pollChallengeStatus) instead of the provided object; ensure you still validate
the resolved object is a PollingCollector (same checks as currently present) and
keep behavior for challenge polling vs continue/timedOut the same as in update()
/ validate() flows.

} catch (err) {
const errorMessage = err instanceof Error ? err.message : String(err);
log.error(errorMessage);
return {
error: {
message: errorMessage || 'An unexpected error occurred during poll operation',
type: 'internal_error',
},
type: 'internal_error',
};
}
},

/**
* @method client - Selector to get the node.client from state
* @returns {Node.client} - the client property from the current node
Expand Down
Loading
Loading