SDKS-5102: Add Metadata, Image, and FIDO2 Error Capabilities - #126
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📝 WalkthroughWalkthroughThe React DaVinci client now supports image and metadata collectors, structured FIDO error display, themed read-only output, and raw OIDC redirect overrides. Documentation and end-to-end coverage were updated. ChangesDaVinci collector updates
FIDO error updates
Client presentation and configuration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant MetadataComponent
participant runThirdPartySdk
participant updater
participant Form
User->>MetadataComponent: Select Success or Failure
MetadataComponent->>runThirdPartySdk: Run with collector config
runThirdPartySdk-->>MetadataComponent: Return success or error
MetadataComponent->>updater: Submit metadata result
updater-->>MetadataComponent: Return update status
MetadataComponent->>Form: Advance after successful update
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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. A rabbit checks each image bright, Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
javascript/reactjs-todo-davinci/client/components/davinci-client/metadata.js (1)
64-81: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAlways clear loading state when an SDK operation rejects.
A rejected third-party SDK call or thrown updater bypasses line 81, leaving both actions disabled with no user-facing error. Wrap the sequence in
try/catch/finally.Proposed fix
async function handleContinue(shouldSucceed) { setIsLoading(true); setError(null); - const sdkResult = await runThirdPartySdk(collector.output.config, shouldSucceed); - const updateResult = - sdkResult && 'error' in sdkResult - ? updater({ code: 'METADATA_PROCESSING_ERROR', message: sdkResult.error }) - : updater(sdkResult.value); - - if (updateResult && 'error' in updateResult) { - setError(updateResult.error?.message || 'Update error'); - console.error('Error updating metadata collector:', updateResult.error); - } else { - await submitForm(); + try { + const sdkResult = await runThirdPartySdk(collector.output.config, shouldSucceed); + const updateResult = + sdkResult && 'error' in sdkResult + ? updater({ code: 'METADATA_PROCESSING_ERROR', message: sdkResult.error }) + : updater(sdkResult.value); + + if (updateResult && 'error' in updateResult) { + setError(updateResult.error?.message || 'Update error'); + } else { + await submitForm(); + } + } catch (error) { + console.error('Error processing metadata collector:', error); + setError('Metadata processing failed'); + } finally { + setIsLoading(false); } - - setIsLoading(false); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@javascript/reactjs-todo-davinci/client/components/davinci-client/metadata.js` around lines 64 - 81, Update handleContinue to wrap the runThirdPartySdk, updater, and submitForm sequence in try/catch/finally. Catch rejected SDK calls or thrown updater errors, surface the failure through setError, and ensure setIsLoading(false) runs in finally so both actions are re-enabled.javascript/reactjs-todo-davinci/e2e/davinci-image.spec.js (1)
15-36: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd policy-independent tests that run in CI.
Both new E2E suites are skipped with
TBDflow IDs, so CI does not exercise image URL filtering or metadata success/failure continuation.
javascript/reactjs-todo-davinci/e2e/davinci-image.spec.js#L15-L36: retain the policy-backed placeholder, but add runnable component coverage for allowed, malformed, and unsafe href values.javascript/reactjs-todo-davinci/e2e/davinci-metadata.spec.js#L15-L36: retain the policy-backed placeholder, but add runnable component coverage for successful updates, returned update errors, and rejected SDK calls.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@javascript/reactjs-todo-davinci/e2e/davinci-image.spec.js` around lines 15 - 36, Add runnable, policy-independent component coverage while retaining the skipped policy-backed tests in javascript/reactjs-todo-davinci/e2e/davinci-image.spec.js lines 15-36: cover allowed, malformed, and unsafe href values. Also add runnable coverage in javascript/reactjs-todo-davinci/e2e/davinci-metadata.spec.js lines 15-36 for successful metadata updates, returned update errors, and rejected SDK calls, using the existing image and metadata test symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@javascript/reactjs-todo-davinci/e2e/davinci-fido.spec.js`:
- Around line 111-116: Update the alert text assertions in the FIDO error test
to avoid requiring an exact match against the alert container, which also
includes the Try Again button. Use toContainText for both failure assertions, or
target the alert’s inner error-message div while preserving the existing
expected messages.
---
Nitpick comments:
In
`@javascript/reactjs-todo-davinci/client/components/davinci-client/metadata.js`:
- Around line 64-81: Update handleContinue to wrap the runThirdPartySdk,
updater, and submitForm sequence in try/catch/finally. Catch rejected SDK calls
or thrown updater errors, surface the failure through setError, and ensure
setIsLoading(false) runs in finally so both actions are re-enabled.
In `@javascript/reactjs-todo-davinci/e2e/davinci-image.spec.js`:
- Around line 15-36: Add runnable, policy-independent component coverage while
retaining the skipped policy-backed tests in
javascript/reactjs-todo-davinci/e2e/davinci-image.spec.js lines 15-36: cover
allowed, malformed, and unsafe href values. Also add runnable coverage in
javascript/reactjs-todo-davinci/e2e/davinci-metadata.spec.js lines 15-36 for
successful metadata updates, returned update errors, and rejected SDK calls,
using the existing image and metadata test symbols.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3f0ae4e5-1d46-45ef-bf00-3105bc033830
📒 Files selected for processing (10)
javascript/reactjs-todo-davinci/README.mdjavascript/reactjs-todo-davinci/client/components/davinci-client/fido.jsjavascript/reactjs-todo-davinci/client/components/davinci-client/form.jsjavascript/reactjs-todo-davinci/client/components/davinci-client/image.jsjavascript/reactjs-todo-davinci/client/components/davinci-client/metadata.jsjavascript/reactjs-todo-davinci/client/components/davinci-client/readonly.jsjavascript/reactjs-todo-davinci/e2e/davinci-fido.spec.jsjavascript/reactjs-todo-davinci/e2e/davinci-image.spec.jsjavascript/reactjs-todo-davinci/e2e/davinci-metadata.spec.jsjavascript/reactjs-todo-davinci/package.json
| */ | ||
| function describeFidoError(fidoError) { | ||
| if (fidoError.type === 'fido_error') { | ||
| return fidoError.message || 'Your device or browser could not complete this request.'; |
There was a problem hiding this comment.
Can we return fidoError.code instead so that we can assert a specific error in the e2e test? For example, if the prompt is cancelled, you should get a NotAllowedError code.
Example:
https://github.com/ForgeRock/ping-javascript-sdk/blob/main/e2e/davinci-suites/src/fido.test.ts#L239
There was a problem hiding this comment.
describeFidoError now returns { message, code }, exposed via data-error-code on the alert. I've added assertions on it in both failure tests, matching the pattern SDK e2e suite.
Thanks!
| setError(describeFidoError(response)); | ||
| console.error('Fido error:', response); |
There was a problem hiding this comment.
If there is an error, we still need to update the collector with that error and send it to DaVinci. Are we doing that here?
https://github.com/ForgeRock/ping-javascript-sdk/blob/main/e2e/davinci-app/components/fido.ts#L33-L37
There was a problem hiding this comment.
Right, the error branch now calls updater(response) and send it to DaVinci.
Thanks for pointing this out.
There was a problem hiding this comment.
Maybe I'm reading it wrong but the error branch still does not appear to submit the error value to DaVinci. i.e. it only calls updater and does not call submitForm(). Perhaps we can refactor the error and success branches. They should both update and call submit form so these bits of logic can be pulled out.
There was a problem hiding this comment.
Yeah, you're right. I now moved the update+submit logic into a shared updateAndSubmit helper used by both branches. The error branch now calls submitForm() as well.
Thanks for pointing this out.
| "@forgerock/oidc-client": "latest", | ||
| "@forgerock/sdk-utilities": "latest", | ||
| "@forgerock/protect": "latest", | ||
| "@forgerock/sdk-utilities": "latest", |
There was a problem hiding this comment.
Why do we need sdk utilities package?
There was a problem hiding this comment.
Yeah, @forgerock/sdk-utilities itself is pre-existing, used for makeOidcConfig/makeDavinciConfig. For some reason it got automatically reordered at some point. I restored it back to how it was before.
| "webpack-dev-server": "^5.1.0" | ||
| }, | ||
| "dependencies": { | ||
| "@forgerock/davinci-client": "latest", |
There was a problem hiding this comment.
If you want to test this with a beta you can grab it from here:
ForgeRock/ping-javascript-sdk#730 (comment)
Please mark this PR with DO NOT MERGE label so we don't accidentally merge it before the 2.2 release.
There was a problem hiding this comment.
Yeah, that's what I've done for local testing. I used pkg.pr.new to install the SDK from that PR.
The "do not merge" label was applied when this PR was created. It'll be merged only after these features are released.
a4a267a to
4d19084
Compare
There was a problem hiding this comment.
The hardcoded redreictUri prevents user configured redirectUri overrides. I found this was an issue while testing. We should move the hardcoded or default value up (i.e., move redirectUri above) so that any spread from rawConfig.oidc (i.e., move rawConfig.oidc below) which is the user defined value takes precedence.
4d19084 to
0488c65
Compare
Nice catch! Moved the default |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
javascript/reactjs-todo-davinci/client/components/davinci-client/fido.js (1)
14-33: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for the
unknown_errorbranch.The changed e2e cases cover only
fido_errorwithNotAllowedError. Add a focused component or unit test that suppliestype: 'unknown_error'and verifies the generic message and preservedcodefromdescribeFidoError.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@javascript/reactjs-todo-davinci/client/components/davinci-client/fido.js` around lines 14 - 33, Add a focused unit or component test for describeFidoError that passes type 'unknown_error' with a code and verifies it returns the generic unexpected-error message while preserving that code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@javascript/reactjs-todo-davinci/e2e/davinci-image.spec.js`:
- Around line 11-27: Replace the placeholder imageFlowAcrValue with a
deterministic DaVinci flow fixture that emits an IMAGE collector whose
output.href uses an unsafe scheme. Update the test to assert the fixture’s
unsafe href is present before verifying no anchor wraps the image, or move the
unsafe-href behavior into a focused component test so the assertion cannot pass
when href is absent.
---
Nitpick comments:
In `@javascript/reactjs-todo-davinci/client/components/davinci-client/fido.js`:
- Around line 14-33: Add a focused unit or component test for describeFidoError
that passes type 'unknown_error' with a code and verifies it returns the generic
unexpected-error message while preserving that code.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9834c6df-e282-4001-bc5e-02b640079057
📒 Files selected for processing (4)
javascript/reactjs-todo-davinci/client/components/davinci-client/fido.jsjavascript/reactjs-todo-davinci/client/constants.jsjavascript/reactjs-todo-davinci/e2e/davinci-fido.spec.jsjavascript/reactjs-todo-davinci/e2e/davinci-image.spec.js
0488c65 to
b48226f
Compare
ancheetah
left a comment
ancheetah
left a comment
There was a problem hiding this comment.
Few more comments on FIDO implementation.
| 'Your device or browser could not complete this request.', | ||
| ); | ||
| // Assert the specific WebAuthn failure reason surfaced by the SDK | ||
| await expect(page.getByRole('alert')).toHaveAttribute('data-error-code', 'NotAllowedError'); |
There was a problem hiding this comment.
Instead of asserting an alert from the application, we should assert that the error node in the flow was reached. In the e2e flow this node should output an error message on the screen with the error code. This tests that we sent the correct error payload to DaVinci and they routed us to an error node.
There was a problem hiding this comment.
Right, this was meant to be a temporary change while the Fido errors PR was still in development. Thanks for catching this.
Updated both failure tests to assert the DaVinci error node's alert instead of FidoComponent's own local one, plus that Try Again disappears once the flow advances. Ran both live against a pkg.pr.new build of #730 and confirmed the literal copy: 'FIDO Registration Error - NotAllowedError' / 'FIDO Authentication Error - NotAllowedError', matching the SDK's own fido.test.ts assertion approach.
b48226f to
a4bafb2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@javascript/reactjs-todo-davinci/client/components/davinci-client/fido.js`:
- Around line 81-82: Update the FIDO error handling around describeFidoError to
pass response.error, the typed error payload, so the correct message and
data-error-code are produced. Keep response unchanged as the argument to
updateAndSubmit.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 29c42400-cafd-4016-8aef-f681b6f84262
📒 Files selected for processing (3)
javascript/reactjs-todo-davinci/client/components/davinci-client/fido.jsjavascript/reactjs-todo-davinci/e2e/davinci-fido.spec.jsjavascript/reactjs-todo-davinci/e2e/davinci-image.spec.js
🚧 Files skipped from review as they are similar to previous changes (2)
- javascript/reactjs-todo-davinci/e2e/davinci-fido.spec.js
- javascript/reactjs-todo-davinci/e2e/davinci-image.spec.js
ancheetah
left a comment
ancheetah
left a comment
There was a problem hiding this comment.
Looks great! Thanks Gabriel
a4bafb2 to
98808cc
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
javascript/reactjs-todo-davinci/client/components/davinci-client/image.js (1)
18-24: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd passing component tests for the URL policy.
The image e2e suite is skipped. Add focused tests for valid
http:andhttps:URLs, rejectedjavascript:,data:, andftp:schemes, and malformed values. These tests do not require a DaVinci policy and will prevent sanitizer regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@javascript/reactjs-todo-davinci/client/components/davinci-client/image.js` around lines 18 - 24, เพิ่ม focused component tests for parseSafeHref covering valid http: and https: URLs, rejecting javascript:, data:, and ftp: schemes, and returning null for malformed values. Keep the tests independent of any DaVinci policy and assert the sanitizer’s accepted or rejected output directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@javascript/reactjs-todo-davinci/client/components/davinci-client/image.js`:
- Around line 18-24: เพิ่ม focused component tests for parseSafeHref covering
valid http: and https: URLs, rejecting javascript:, data:, and ftp: schemes, and
returning null for malformed values. Keep the tests independent of any DaVinci
policy and assert the sanitizer’s accepted or rejected output directly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 821f5d8e-0ad2-4a10-86f7-a9de04d79643
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
javascript/reactjs-todo-davinci/README.mdjavascript/reactjs-todo-davinci/client/components/davinci-client/fido.jsjavascript/reactjs-todo-davinci/client/components/davinci-client/form.jsjavascript/reactjs-todo-davinci/client/components/davinci-client/image.jsjavascript/reactjs-todo-davinci/client/components/davinci-client/metadata.jsjavascript/reactjs-todo-davinci/client/components/davinci-client/readonly.jsjavascript/reactjs-todo-davinci/client/constants.jsjavascript/reactjs-todo-davinci/e2e/davinci-fido.spec.jsjavascript/reactjs-todo-davinci/e2e/davinci-image.spec.js
🚧 Files skipped from review as they are similar to previous changes (6)
- javascript/reactjs-todo-davinci/README.md
- javascript/reactjs-todo-davinci/client/constants.js
- javascript/reactjs-todo-davinci/client/components/davinci-client/form.js
- javascript/reactjs-todo-davinci/e2e/davinci-fido.spec.js
- javascript/reactjs-todo-davinci/client/components/davinci-client/readonly.js
- javascript/reactjs-todo-davinci/e2e/davinci-image.spec.js
The changes I requested have been addressed.
I did a cursory review of the PR but didn't go into details, so dismissing my review.
ancheetah
left a comment
ancheetah
left a comment
There was a problem hiding this comment.
Some new suggestions for the FIDO component
| * Details: `fidoClient.register`/`authenticate` return a typed | ||
| * `GenericError` on failure. Its `type` field ('fido_error' vs | ||
| * 'unknown_error') lets the flow distinguish an expected WebAuthn/browser | ||
| * failure from an unexpected internal one, rather than parsing a message | ||
| * string. `code` (e.g. `NotAllowedError`) is surfaced as a data attribute | ||
| * so e2e tests can assert on the specific WebAuthn failure reason. |
There was a problem hiding this comment.
Technically the only type of GenericError the FIDO API can return is fido_error I believe. Also, let's not refer to it as a "client", that was a mistake in my typing. Let's call it an API. I would also remove the mention of e2e tests and have something like this:
The FIDO API register() and authenticate() methods return a GenericError on failure with type fido_error. The error code determines if it was a DOM exception (e.g. NotAllowedError) vs internal error (UnknownError). You may choose to handle this error client side, or send the error to DaVinci to reach an error branch configured in your flow. To send the error to DaVinci, update the collector with the error and submit it by calling davincClient.next().
There was a problem hiding this comment.
Done. Rewrote this comment with your suggested wording, and updated describeFidoError's JSDoc to match.
Thanks!
| @@ -24,6 +44,16 @@ export default function FidoComponent({ collector, updater, submitForm }) { | |||
| const [hasAttempted, setHasAttempted] = useState(false); // for registration auto-trigger | |||
| const fidoClient = fido(); | |||
There was a problem hiding this comment.
Can we rename fidoClient to fidoApi?
There was a problem hiding this comment.
Updated. Thanks!
ancheetah
left a comment
ancheetah
left a comment
There was a problem hiding this comment.
FIDO changes look good. Thanks Gabriel!
feat(reactjs-todo-davinci): branch FIDO2 error UI on typed error contract chore(reactjs-todo-davinci): temporarily depend on davinci-client PR #727 build for MetadataCollector feat(reactjs-todo-davinci): add MetadataCollector component test(reactjs-todo-davinci): add image/metadata e2e specs, extend fido error assertion fix(reactjs-todo-davinci): import getMetadataError from SDK utils subpath fix(reactjs-todo-davinci): sanitize ImageCollector href scheme before render fix(reactjs-todo-davinci): build MetadataError literal, drop removed getMetadataError import fix(reactjs-todo-davinci): redact metadata payload display, flag unsafe storage of sensitive data test(reactjs-todo-davinci): assert positive fido_error copy instead of negative check fix(reactjs-todo-davinci): log error details with labeled console.error, matching codebase convention docs(reactjs-todo-davinci): add ImageCollector, MetadataCollector to README feat(reactjs-todo-davinci): redesign MetadataCollector around third-party SDK invocation fix(e2e): use toContainText for fido alert assertions fix(davinci): report fido error to updater before local display chore(reactjs-todo-davinci): revert unnecessary dependency reorder test(reactjs-todo-davinci): trim e2e specs to sample-specific coverage, drop duplicate metadata spec fix(davinci): submit fido error to davinci via shared update-and-submit helper test(e2e): assert davinci error node reached with literal error copy
c4e72bf to
2aefb3c
Compare
4 participants
Summary
https://pingidentity.atlassian.net/browse/SDKS-5102
Updates the
reactjs-todo-davincisample app to demonstrate three new@forgerock/davinci-clientcapabilities: theMetadataCollector(pausing a DaVinci flow to invoke a third-party SDK), theImageCollector(Forms image rendering), and the typed FIDO2 client error contract. Each is added as a reference example for developers integrating DaVinci flows.Changes
reactjs-todo-davinci/client/components/davinci-clientmetadata.js(new) —MetadataComponentforMetadataCollector. Calls arunThirdPartySdk(config)stand-in againstcollector.output.config, then reports that SDK's outcome back to DaVinci: its success value viaupdater(sdkResult.value), or a structuredMetadataError({code, message}object literal, since the SDK exposesMetadataErroras a type with no builder function) on failure. Checksupdater's own return for an error before callingsetNext(). Renders explicit "Success" and "Failure" buttons so both branches have a deterministic trigger, mirroring the SDK repo's own e2e fixture.image.js(new) —ImageComponentforImageCollector, rendering<img src alt data-testid="form-image">, wrapped in<a href>whenoutput.hrefis present.parseSafeHrefrestricts the href tohttp:/https:schemes, as the SDK's type doc requires consumers to sanitize this value.fido.js— newdescribeFidoErrorhelper branches displayed copy on the typedGenericError.typereturned byfido().register()/authenticate():fido_errorpasses the SDK message through as an expected WebAuthn/browser failure, anything else gets generic unexpected-error copy. Error logging now uses labeledconsole.errorcalls.form.js—ImageCollectorandMetadataCollectorcases added tomapCollectorsToComponents, following the existing switch-statement pattern.readonly.js— appliestheme.textClassandmb-3to both theReadOnlyCollectorandRichTextCollectorrender branches. Found during manual testing: the metadata flow's trailing message step rendered withclass="", unstyled and invisible against the dark theme.reactjs-todo-davinci(docs / manifest)README.md—ImageCollectorandMetadataCollectoradded to the supported-collectors list.package.json— dependency keys reordered alphabetically.@forgerock/davinci-clientstays at"latest".Tests
e2e/davinci-image.spec.js(new) — asserts the image renders with non-emptysrc/altand no hyperlink wrapper, and that the wrapper appears whenoutput.hrefis present.test.describe.skip.e2e/davinci-metadata.spec.js(new) — asserts the flow advances on the Success path and on the Failure path (structured error reported, flow still advances).test.describe.skip.e2e/davinci-fido.spec.js— the two existing failure tests now assert the branchedfido_errorcopy directly rather than only checking the generic fallback is absent. Remainstest.describe.skipfor the same pre-existing WebAuthn registration limitation.Unverified coverage — please read before approving
The image and metadata collector paths are not covered by a passing e2e run. Both new specs are
test.describe.skipwithacrValue = 'TBD', because they require DaVinci flow policy IDs that emit anIMAGEfield and aMETADATAaction, which are not yet available. A green CI run on this PR does not exercise either new component. The FIDO2 spec is likewise still skipped (pre-existing).The metadata component was verified manually against a real Metadata Flow, which is how the
readonly.jsstyling bug was found. The image component has not been verified against a live flow.MetadataCollectoris not in a published@forgerock/davinci-clientrelease. It only exists on PR #727 (SDKS-5100-metadata-collector); the latest published version is2.1.0.package.jsondeclares"latest"and the lockfile resolves entirely to the npm registry, sonpm ciis clean and no ephemeral build URL is committed. To exercise the metadata path locally before #727 merges, apply an uncommitted override:Revert that before committing. Once #727 publishes,
"latest"resolves correctly with no change needed here.How to test
1. Metadata collector
npm cifrom the repo root, then apply thepkg.pr.newoverride above.npm run start:reactjs-todo-dvfrom/javascript.?acrValue=<metadata-flow-policy-id>and sign in.<pre>block.updaterreports it, and the flow advances.METADATA_PROCESSING_ERRORis reported viaupdaterand the flow still advances. Confirm the trailing message step is legible (this is thereadonly.jsfix).2. Image collector
?acrValue=<image-flow-policy-id>and sign in.srcandalt.href, confirm the image is wrapped in a same-tab link.Verify the href sanitizer
Configure a flow whose image
hrefuses ajavascript:scheme, or temporarily hardcode one. Confirm the image renders unwrapped rather than as a link.3. FIDO2 typed error branching
fido_error, not the generic "Something unexpected went wrong." copy.Summary by CodeRabbit
New Features
Bug Fixes
Documentation