feat(davinci-client): add polling support (SDKS-4687)#563
Open
feat(davinci-client): add polling support (SDKS-4687)#563
Conversation
|
📝 WalkthroughWalkthroughThis pull request introduces a new polling feature to the DaVinci client, enabling support for long-running operations with challenge status checking and retry logic. It adds a Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UI Component
participant Client as DaVinci Client
participant API as RTK Query API
participant Server as DaVinci Server
UI->>Client: poll(collector)
activate Client
alt Challenge Polling Branch
Client->>Client: validateChallenge()
Client->>API: dispatch poll.initiate()
activate API
API->>Server: POST /poll (with interactionId)
activate Server
Server-->>API: Polling Response
deactivate Server
loop Until Complete or Timeout
API->>Server: Check isChallengeComplete
Server-->>API: Status Update
alt Challenge Expired
API-->>Client: 'expired'
else Challenge Complete
API-->>Client: 'approved'|'denied'|'continue'
else Retries Exhausted
API-->>Client: 'timedOut'
end
end
deactivate API
else Continue Polling Branch
Client->>Client: Check retriesRemaining
loop Until Retries Exhausted
Client->>Client: Wait pollInterval
Client-->>Client: Return 'continue'
end
Client-->>Client: Return 'timedOut'
end
deactivate Client
Client-->>UI: PollingStatus | InternalErrorResponse
alt Success
UI->>UI: Display Result
UI->>Client: updater(status)
UI->>UI: submitForm()
else Error
UI->>UI: Display Error Message
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Contributor
|
View your CI Pipeline Execution ↗ for commit e5ad583
☁️ Nx Cloud last updated this comment at |
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (14.10%) 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 #563 +/- ##
===========================================
- Coverage 70.90% 14.82% -56.09%
===========================================
Files 53 153 +100
Lines 2021 26597 +24576
Branches 377 1065 +688
===========================================
+ Hits 1433 3942 +2509
- Misses 588 22655 +22067
🚀 New features to boost your workflow:
|
@forgerock/davinci-client
@forgerock/device-client
@forgerock/journey-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: |
Contributor
|
Deployed f6f706c to https://ForgeRock.github.io/ping-javascript-sdk/pr-563/f6f706c691d95b4724bd09b488b01e75bbcb117e branch gh-pages in ForgeRock/ping-javascript-sdk |
Contributor
📦 Bundle Size Analysis📦 Bundle Size Analysis🚨 Significant Changes🔻 @forgerock/device-client - 0.0 KB (-9.7 KB, -100.0%) 📊 Minor Changes📉 @forgerock/device-client - 9.7 KB (-0.0 KB) ➖ No Changes➖ @forgerock/oidc-client - 24.9 KB 14 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
ancheetah
force-pushed
the
SDKS-4687-polling
branch
from
764084e to
8d0d79e
Compare
ancheetah
force-pushed
the
SDKS-4687-polling
branch
2 times, most recently
from
5429843 to
62cc17a
Compare
ancheetah
force-pushed
the
SDKS-4687-polling
branch
from
62cc17a to
e5ad583
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/actions/setup/action.yml:
- Around line 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.
In `@e2e/davinci-app/components/polling.ts`:
- Around line 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.
In `@packages/davinci-client/src/lib/client.store.ts`:
- Around line 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.
In `@packages/davinci-client/src/lib/client.store.utils.ts`:
- Line 176: The URL is built using an opaque challenge token into the path via
the variable challengeEndpoint, which can break if challenge contains characters
like '/', '?', '#', or '%'; fix by URL-encoding the token before interpolation
(e.g., create an encodedChallenge = encodeURIComponent(challenge) and use that
in the template for challengeEndpoint) so the path is always correctly
constructed when using the challenge variable.
- Around line 130-173: The returned error objects in the challenge-polling logic
(checks around serverSlice.status, links/self href parsing, baseUrl/envId
construction, and interactionId) are missing the discriminant property required
by the response union; update each return that currently yields { error: {
message: ..., type: ... } } (near the checks using serverSlice, links/self.href,
baseUrl/envId, and interactionId) to include the top-level type discriminator
(e.g., add type: 'internal_error' or the appropriate union tag to the outer
object) so the shape becomes { type: 'internal_error', error: { ... } } (or the
correct discriminator value for the specific case) instead of relying on the
cast to InternalErrorResponse.
- Around line 193-245: The handler and loop predicate assume data and error.data
are objects; add runtime guards to avoid crashes and incorrect polling: in the
while predicate (the Micro... while: callback) check that data is a non-null
object before accessing ['isChallengeComplete'] (treat non-objects as an
immediate failure), and inside the flatMap handler guard pollResponse and
error.data with typeof x === 'object' && x !== null before indexing; if these
guards fail, return Micro.fail(...) with an InternalErrorResponse (or
Micro.succeed('error') where appropriate) so bad/non-object responses fail fast
instead of continuing to poll. Reference symbols: the while predicate, flatMap
callback, pollResponse, and the error handling branch that reads
error.data/serviceName.
In `@packages/davinci-client/src/lib/davinci.api.ts`:
- Around line 61-66: The PR is missing regression tests for the poll-only
request behavior: add tests that call the request flow for the 'poll' endpoint
and assert that prepareHeaders does not set 'x-requested-with' or
'x-requested-platform' (only 'Accept' remains) and separately assert that the
normal onQueryStarted state-update flow is not executed for poll (e.g., spy/mock
on the onQueryStarted handler and verify it is not called or that the
store/state remains unchanged). Target the functions prepareHeaders and the
query handler that triggers onQueryStarted (mock the client or middleware used
by the Davinci API) so the tests cover both header-skipping and poll bypass
behavior and increase patch coverage for davinci.api.ts.
In `@packages/davinci-client/src/lib/davinci.types.ts`:
- Around line 215-221: PollingField currently requires challenge but the code
uses two polling modes; change the type to reflect that challenge is optional or
use a discriminated union: update the PollingField definition (symbol:
PollingField) so that challenge is optional (challenge?: string) or split into
two types discriminated by pollChallengeStatus (e.g., PollingFieldChallenge and
PollingFieldContinue) so the shape matches runtime checks around
pollChallengeStatus and challenge (see usage in collector.utils and
client.store) and aligns with PollingOutputValue which already marks those
fields optional.
In `@packages/davinci-client/src/lib/davinci.utils.ts`:
- Around line 66-69: The current logic that sets eventType by checking
collectors.some(collector => collector.type === 'PollingCollector') wrongly
classifies submits whenever any PollingCollector exists; change this to either
(a) accept an explicit intent/flag from the caller (preferred) and use that to
set eventType, or (b) if keeping local detection, gate on a populated polling
payload (e.g., check collector.type === 'PollingCollector' &&
collector.input/values are non-empty) instead of mere presence; update the
eventType assignment and any similar logic around the collectors.some(...) check
(also change the related check at the other occurrence around line 78) to use
the chosen approach so mixed-node submissions are routed correctly.
In `@packages/davinci-client/src/lib/node.reducer.ts`:
- Around line 339-347: The reducer currently throws when retriesRemaining is
falsy, which treats 0 as missing; change the checks so you first verify the
collector exists (const pollCollector = state.find(...); if (!pollCollector)
throw ...) and then explicitly check for retriesRemaining being undefined (if
(pollCollector.output.config.retriesRemaining === undefined) throw ...); finally
decrement safely by either doing if
(pollCollector.output.config.retriesRemaining > 0) {
pollCollector.output.config.retriesRemaining--; } else {
pollCollector.output.config.retriesRemaining = 0; } or set retriesRemaining =
Math.max(0, retriesRemaining - 1) to clamp to zero (referencing
pollCollectorValues, pollCollector, and
pollCollector.output.config.retriesRemaining).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c9ba0fbb-333f-4a89-a914-c1c75cb69b7c
📒 Files selected for processing (20)
.github/actions/setup/action.yml.github/workflows/ci.ymle2e/davinci-app/components/polling.tse2e/davinci-app/main.tspackage.jsonpackages/davinci-client/src/lib/client.store.tspackages/davinci-client/src/lib/client.store.utils.tspackages/davinci-client/src/lib/client.types.tspackages/davinci-client/src/lib/collector.types.tspackages/davinci-client/src/lib/collector.utils.test.tspackages/davinci-client/src/lib/collector.utils.tspackages/davinci-client/src/lib/davinci.api.tspackages/davinci-client/src/lib/davinci.types.tspackages/davinci-client/src/lib/davinci.utils.tspackages/davinci-client/src/lib/node.reducer.tspackages/davinci-client/src/lib/node.slice.tspackages/davinci-client/src/lib/node.types.test-d.tspackages/davinci-client/src/lib/node.types.tspackages/davinci-client/src/types.tspackages/sdk-effects/sdk-request-middleware/src/lib/request-mware.derived.ts
Comment on lines
+34
to
+36
| run: | | ||
| corepack enable | ||
| corepack install -g npm@latest |
There was a problem hiding this comment.
🧩 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/ -nRepository: 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 1Repository: 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:36—corepack install -g npm@latest.github/workflows/publish.yml:54—npm 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.
Comment on lines
+27
to
+57
| 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(); | ||
| }; |
There was a problem hiding this comment.
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.
Comment on lines
+420
to
+480
| 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', | ||
| }; | ||
| } |
There was a problem hiding this comment.
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.
Comment on lines
+130
to
+173
| if (serverSlice.status !== 'continue') { | ||
| return { | ||
| error: { | ||
| message: 'Not in a continue node state, must be in a continue node to use poll method', | ||
| type: 'state_error', | ||
| }, | ||
| } as InternalErrorResponse; | ||
| } | ||
|
|
||
| // Construct the challenge polling endpoint | ||
| const links = serverSlice._links; | ||
| if (!links || !('self' in links) || !('href' in links['self']) || !links['self'].href) { | ||
| return { | ||
| error: { | ||
| message: 'No self link found in server info for challenge polling operation', | ||
| type: 'internal_error', | ||
| }, | ||
| } as InternalErrorResponse; | ||
| } | ||
|
|
||
| const selfUrl = links['self'].href; | ||
| const url = new URL(selfUrl); | ||
| const baseUrl = url.origin; | ||
| const paths = url.pathname.split('/'); | ||
| const envId = paths[1]; | ||
|
|
||
| if (!baseUrl || !envId) { | ||
| return { | ||
| error: { | ||
| message: | ||
| 'Failed to construct challenge polling endpoint. Requires host and environment ID.', | ||
| type: 'parse_error', | ||
| }, | ||
| } as InternalErrorResponse; | ||
| } | ||
|
|
||
| const interactionId = serverSlice.interactionId; | ||
| if (!interactionId) { | ||
| return { | ||
| error: { | ||
| message: 'Missing interactionId in server info for challenge polling', | ||
| type: 'internal_error', | ||
| }, | ||
| } as InternalErrorResponse; |
There was a problem hiding this comment.
Add the missing type discriminator to these error returns.
The casts hide a runtime bug here: Lines 131, 142, 157, and 168 all return { error: ... } without type: 'internal_error', so downstream discriminated-union checks will miss these failures.
💡 Suggested fix
if (serverSlice.status !== 'continue') {
return {
error: {
message: 'Not in a continue node state, must be in a continue node to use poll method',
type: 'state_error',
},
- } as InternalErrorResponse;
+ type: 'internal_error',
+ };
}
@@
if (!links || !('self' in links) || !('href' in links['self']) || !links['self'].href) {
return {
error: {
message: 'No self link found in server info for challenge polling operation',
type: 'internal_error',
},
- } as InternalErrorResponse;
+ type: 'internal_error',
+ };
}
@@
if (!baseUrl || !envId) {
return {
error: {
message:
'Failed to construct challenge polling endpoint. Requires host and environment ID.',
type: 'parse_error',
},
- } as InternalErrorResponse;
+ type: 'internal_error',
+ };
}
@@
if (!interactionId) {
return {
error: {
message: 'Missing interactionId in server info for challenge polling',
type: 'internal_error',
},
- } as InternalErrorResponse;
+ type: 'internal_error',
+ };
}🤖 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.utils.ts` around lines 130 -
173, The returned error objects in the challenge-polling logic (checks around
serverSlice.status, links/self href parsing, baseUrl/envId construction, and
interactionId) are missing the discriminant property required by the response
union; update each return that currently yields { error: { message: ..., type:
... } } (near the checks using serverSlice, links/self.href, baseUrl/envId, and
interactionId) to include the top-level type discriminator (e.g., add type:
'internal_error' or the appropriate union tag to the outer object) so the shape
becomes { type: 'internal_error', error: { ... } } (or the correct discriminator
value for the specific case) instead of relying on the cast to
InternalErrorResponse.
| } as InternalErrorResponse; | ||
| } | ||
|
|
||
| const challengeEndpoint = `${baseUrl}/${envId}/davinci/user/credentials/challenge/${challenge}/status`; |
There was a problem hiding this comment.
URL-encode the challenge token before putting it in the path.
challenge is opaque input. If it ever contains /, ?, #, or %, this builds a different URL than intended and the poll will hit the wrong route.
💡 Suggested fix
- const challengeEndpoint = `${baseUrl}/${envId}/davinci/user/credentials/challenge/${challenge}/status`;
+ const encodedChallenge = encodeURIComponent(challenge);
+ const challengeEndpoint =
+ `${baseUrl}/${envId}/davinci/user/credentials/challenge/${encodedChallenge}/status`;📝 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
| const challengeEndpoint = `${baseUrl}/${envId}/davinci/user/credentials/challenge/${challenge}/status`; | |
| const encodedChallenge = encodeURIComponent(challenge); | |
| const challengeEndpoint = | |
| `${baseUrl}/${envId}/davinci/user/credentials/challenge/${encodedChallenge}/status`; |
🤖 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.utils.ts` at line 176, The URL
is built using an opaque challenge token into the path via the variable
challengeEndpoint, which can break if challenge contains characters like '/',
'?', '#', or '%'; fix by URL-encoding the token before interpolation (e.g.,
create an encodedChallenge = encodeURIComponent(challenge) and use that in the
template for challengeEndpoint) so the path is always correctly constructed when
using the challenge variable.
Comment on lines
+193
to
+245
| while: ({ data, error }) => | ||
| retriesLeft > 0 && !error && !(data as Record<string, unknown>)['isChallengeComplete'], | ||
| schedule: Micro.scheduleSpaced(pollInterval), | ||
| }).pipe( | ||
| Micro.flatMap(({ data, error }) => { | ||
| const pollResponse = data as Record<string, unknown>; | ||
|
|
||
| // Check for any errors and return the appropriate status | ||
| if (error) { | ||
| // SerializedError | ||
| let message = 'An unknown error occurred while challenge polling'; | ||
| if ('message' in error && error.message) { | ||
| message = error.message; | ||
| return Micro.fail({ | ||
| error: { | ||
| message, | ||
| type: 'unknown_error', | ||
| }, | ||
| type: 'internal_error', | ||
| } as InternalErrorResponse); | ||
| } | ||
|
|
||
| // FetchBaseQueryError | ||
| let status: number | string = 'unknown'; | ||
| if ('status' in error) { | ||
| status = error.status; | ||
|
|
||
| const errorDetails = error.data as Record<string, unknown>; | ||
| const serviceName = errorDetails['serviceName']; | ||
|
|
||
| // Check for an expired challenge | ||
| if (status === 400 && serviceName && serviceName === 'challengeExpired') { | ||
| log.debug('Challenge expired for polling'); | ||
| return Micro.succeed('expired' as PollingStatus); | ||
| } else { | ||
| // If we're here there is some other type of network error and status != 200 | ||
| // e.g. A bad challenge can return a httpStatus of 400 with code 4019 | ||
| log.debug('Network error occurred during polling'); | ||
| return Micro.succeed('error' as PollingStatus); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // If a successful response is recieved it can be either a timeout or true success | ||
| if (pollResponse['isChallengeComplete'] === true) { | ||
| const pollStatus = pollResponse['status']; | ||
| if (!pollStatus) { | ||
| return Micro.succeed('error' as PollingStatus); | ||
| } else { | ||
| return Micro.succeed(pollStatus as PollingStatus); | ||
| } | ||
| } else if (retriesLeft <= 0 && !pollResponse['isChallengeComplete']) { | ||
| return Micro.succeed('timedOut' as PollingStatus); |
There was a problem hiding this comment.
Guard data and error.data before treating them as objects.
poll returns unknown, but the loop predicate and handler index straight into data/error.data. Empty or non-object bodies will either throw or get treated as “keep polling” until timeout instead of failing fast on a bad response.
💡 Suggested fix
+ const isRecord = (value: unknown): value is Record<string, unknown> =>
+ typeof value === 'object' && value !== null;
+
const challengePollµ = Micro.repeat(queryµ, {
while: ({ data, error }) =>
- retriesLeft > 0 && !error && !(data as Record<string, unknown>)['isChallengeComplete'],
+ retriesLeft > 0 && !error && isRecord(data) && data['isChallengeComplete'] !== true,
schedule: Micro.scheduleSpaced(pollInterval),
}).pipe(
Micro.flatMap(({ data, error }) => {
- const pollResponse = data as Record<string, unknown>;
+ if (!isRecord(data)) {
+ return Micro.fail({
+ error: {
+ message: 'Malformed polling response',
+ type: 'parse_error',
+ },
+ type: 'internal_error',
+ } as InternalErrorResponse);
+ }
+ const pollResponse = data;
@@
- const errorDetails = error.data as Record<string, unknown>;
- const serviceName = errorDetails['serviceName'];
+ const errorDetails = isRecord(error.data) ? error.data : undefined;
+ const serviceName =
+ typeof errorDetails?.['serviceName'] === 'string'
+ ? errorDetails['serviceName']
+ : undefined;🤖 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.utils.ts` around lines 193 -
245, The handler and loop predicate assume data and error.data are objects; add
runtime guards to avoid crashes and incorrect polling: in the while predicate
(the Micro... while: callback) check that data is a non-null object before
accessing ['isChallengeComplete'] (treat non-objects as an immediate failure),
and inside the flatMap handler guard pollResponse and error.data with typeof x
=== 'object' && x !== null before indexing; if these guards fail, return
Micro.fail(...) with an InternalErrorResponse (or Micro.succeed('error') where
appropriate) so bad/non-object responses fail fast instead of continuing to
poll. Reference symbols: the while predicate, flatMap callback, pollResponse,
and the error handling branch that reads error.data/serviceName.
Comment on lines
+61
to
+66
| prepareHeaders: (headers, { endpoint }) => { | ||
| headers.set('Accept', 'application/json'); | ||
| headers.set('x-requested-with', 'ping-sdk'); | ||
| headers.set('x-requested-platform', 'javascript'); | ||
| if (endpoint !== 'poll') { | ||
| headers.set('x-requested-with', 'ping-sdk'); | ||
| headers.set('x-requested-platform', 'javascript'); | ||
| } |
There was a problem hiding this comment.
Add regression tests for the poll-only request behavior.
This path now relies on two special cases: prepareHeaders must skip the x-requested-* headers for poll, and poll must not run the normal onQueryStarted state-update flow. Codecov is already reporting 11.19% patch coverage with davinci.api.ts among the misses, so this is shipping without a guardrail today.
Also applies to: 432-462
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/davinci-client/src/lib/davinci.api.ts` around lines 61 - 66, The PR
is missing regression tests for the poll-only request behavior: add tests that
call the request flow for the 'poll' endpoint and assert that prepareHeaders
does not set 'x-requested-with' or 'x-requested-platform' (only 'Accept'
remains) and separately assert that the normal onQueryStarted state-update flow
is not executed for poll (e.g., spy/mock on the onQueryStarted handler and
verify it is not called or that the store/state remains unchanged). Target the
functions prepareHeaders and the query handler that triggers onQueryStarted
(mock the client or middleware used by the Davinci API) so the tests cover both
header-skipping and poll bypass behavior and increase patch coverage for
davinci.api.ts.
Comment on lines
+215
to
+221
| export type PollingField = { | ||
| type: 'POLLING'; | ||
| key: string; | ||
| pollInterval: number; | ||
| pollRetries: number; | ||
| pollChallengeStatus: boolean; | ||
| challenge: string; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C3 "type:\s*'POLLING'|pollChallengeStatus|challenge" packages/davinci-client/src e2e/davinci-appRepository: ForgeRock/ping-javascript-sdk
Length of output: 24659
Make challenge optional in PollingField to match two-mode polling semantics.
The PollingField type requires challenge unconditionally, but the codebase uses two distinct polling modes:
- Challenge polling (
pollChallengeStatus: true): requires achallengetoken - Continue polling (
pollChallengeStatus: false): must NOT includechallenge, usesretriesRemaininginstead
Evidence: collector.utils.ts:328-329 explicitly spreads challenge only when present and retriesRemaining when absent. client.store.ts:436-444 routes based on challenge && pollChallengeStatus === true vs !challenge && !pollChallengeStatus. The corrected PollingOutputValue interface already marks both as optional.
Either make challenge?: string in PollingField or use a discriminated union with separate types for each polling mode to reflect the actual contract.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/davinci-client/src/lib/davinci.types.ts` around lines 215 - 221,
PollingField currently requires challenge but the code uses two polling modes;
change the type to reflect that challenge is optional or use a discriminated
union: update the PollingField definition (symbol: PollingField) so that
challenge is optional (challenge?: string) or split into two types discriminated
by pollChallengeStatus (e.g., PollingFieldChallenge and PollingFieldContinue) so
the shape matches runtime checks around pollChallengeStatus and challenge (see
usage in collector.utils and client.store) and aligns with PollingOutputValue
which already marks those fields optional.
Comment on lines
+66
to
+69
| // Set eventType based on PollingCollector presence | ||
| const eventType = collectors?.some((collector) => collector.type === 'PollingCollector') | ||
| ? 'polling' | ||
| : 'submit'; |
There was a problem hiding this comment.
Don't infer eventType from collector presence alone.
This flips every submit on a node to polling as soon as a PollingCollector exists, even if the user is taking a normal submit path or the polling collector still has an empty input. That can mis-route mixed-node submissions. Prefer passing the intent from the caller, or at minimum gate on a populated polling value instead of mere presence.
💡 Proposed fix
- const eventType = collectors?.some((collector) => collector.type === 'PollingCollector')
+ const eventType = collectors?.some(
+ (collector) => collector.type === 'PollingCollector' && collector.input.value,
+ )
? 'polling'
: 'submit';Also applies to: 78-78
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/davinci-client/src/lib/davinci.utils.ts` around lines 66 - 69, The
current logic that sets eventType by checking collectors.some(collector =>
collector.type === 'PollingCollector') wrongly classifies submits whenever any
PollingCollector exists; change this to either (a) accept an explicit
intent/flag from the caller (preferred) and use that to set eventType, or (b) if
keeping local detection, gate on a populated polling payload (e.g., check
collector.type === 'PollingCollector' && collector.input/values are non-empty)
instead of mere presence; update the eventType assignment and any similar logic
around the collectors.some(...) check (also change the related check at the
other occurrence around line 78) to use the chosen approach so mixed-node
submissions are routed correctly.
Comment on lines
+339
to
+347
| .addCase(pollCollectorValues, (state) => { | ||
| // For challenge polling, track and decrement retries when this reducer is called | ||
| const pollCollector = state.find((collector) => collector.type === 'PollingCollector'); | ||
|
|
||
| if (!pollCollector?.output.config.retriesRemaining) { | ||
| throw new Error('No poll collector found to update'); | ||
| } | ||
|
|
||
| pollCollector.output.config.retriesRemaining--; |
There was a problem hiding this comment.
Treat 0 retries as exhausted, not as missing state.
!pollCollector?.output.config.retriesRemaining also trips when the counter is already 0, so an extra continue-poll transition will throw from the reducer instead of leaving the collector in a valid exhausted state. Check for a missing collector and undefined separately, then clamp the decrement.
💡 Proposed fix
.addCase(pollCollectorValues, (state) => {
// For challenge polling, track and decrement retries when this reducer is called
const pollCollector = state.find((collector) => collector.type === 'PollingCollector');
- if (!pollCollector?.output.config.retriesRemaining) {
- throw new Error('No poll collector found to update');
+ if (!pollCollector) {
+ throw new Error('No poll collector found to update');
}
- pollCollector.output.config.retriesRemaining--;
+ if (pollCollector.output.config.retriesRemaining === undefined) {
+ throw new Error('Polling collector does not track retriesRemaining');
+ }
+
+ pollCollector.output.config.retriesRemaining = Math.max(
+ 0,
+ pollCollector.output.config.retriesRemaining - 1,
+ );
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/davinci-client/src/lib/node.reducer.ts` around lines 339 - 347, The
reducer currently throws when retriesRemaining is falsy, which treats 0 as
missing; change the checks so you first verify the collector exists (const
pollCollector = state.find(...); if (!pollCollector) throw ...) and then
explicitly check for retriesRemaining being undefined (if
(pollCollector.output.config.retriesRemaining === undefined) throw ...); finally
decrement safely by either doing if
(pollCollector.output.config.retriesRemaining > 0) {
pollCollector.output.config.retriesRemaining--; } else {
pollCollector.output.config.retriesRemaining = 0; } or set retriesRemaining =
Math.max(0, retriesRemaining - 1) to clamp to zero (referencing
pollCollectorValues, pollCollector, and
pollCollector.output.config.retriesRemaining).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
JIRA Ticket
https://pingidentity.atlassian.net/browse/SDKS-4687
Description
poll()method ondavinciClientSummary by CodeRabbit
Release Notes
New Features
Chores