SDKS-5102: Add Metadata, Image, and FIDO2 Error Capabilities - #126
SDKS-5102: Add Metadata, Image, and FIDO2 Error Capabilities#126SteinGabriel wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 46 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThe React DaVinci client now supports image and metadata collectors, including safe image links and metadata continuation actions. FIDO error messages use typed SDK errors, read-only content adopts theme styling, and related E2E coverage and documentation were updated. ChangesDaVinci collector updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant MetadataComponent
participant ThirdPartySDK
participant CollectorUpdater
participant FormFlow
User->>MetadataComponent: Select Success or Failure
MetadataComponent->>ThirdPartySDK: Run with metadata config
ThirdPartySDK-->>MetadataComponent: Return success or error
MetadataComponent->>CollectorUpdater: Submit result
CollectorUpdater-->>MetadataComponent: Return update status
MetadataComponent->>FormFlow: Advance on success
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![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.
| "@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.
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
2 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
Tests