fix(davinci-client): return-url-from-external-idp #322

ryanbas21 · 2025-06-09T17:41:09Z

JIRA Ticket

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

Description

updating the api for the social login external idp method. Instead of redirecting for the application, we will return a url for the application to redirect to themselves.

changeset-bot · 2025-06-09T17:41:12Z

🦋 Changeset detected

Latest commit: 609d381

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 9 packages

Name	Type
@forgerock/davinci-client	Patch
@forgerock/oidc-client	Patch
@forgerock/sdk-types	Patch
@forgerock/sdk-utilities	Patch
@forgerock/iframe-manager	Patch
@forgerock/sdk-logger	Patch
@forgerock/sdk-oidc	Patch
@forgerock/sdk-request-middleware	Patch
@forgerock/storage	Patch

Not sure what this means? Click here to learn what changesets are.

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

nx-cloud · 2025-06-09T17:41:46Z

View your CI Pipeline Execution ↗ for commit 609d381.

Command	Status	Duration	Result
`nx affected -t build typecheck lint test e2e-ci`	✅ Succeeded	1m 24s	View ↗
`nx-cloud record -- nx format:check`	✅ Succeeded	1s	View ↗

☁️ Nx Cloud last updated this comment at 2025-06-10 22:52:02 UTC

codecov-commenter · 2025-06-09T17:45:24Z

Codecov Report

Attention: Patch coverage is 0% with 52 lines in your changes missing coverage. Please review.

Project coverage is 48.16%. Comparing base (4ec5c25) to head (b90fccd).

Files with missing lines	Patch %	Lines
packages/davinci-client/src/lib/client.store.ts	0.00%	42 Missing ⚠️
packages/davinci-client/src/lib/davinci.api.ts	0.00%	10 Missing ⚠️

❌ Your patch status has failed because the patch coverage (0.00%) 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     #322      +/-   ##
==========================================
- Coverage   50.32%   48.16%   -2.16%     
==========================================
  Files          29       29              
  Lines        1713     1715       +2     
  Branches      194      192       -2     
==========================================
- Hits          862      826      -36     
- Misses        851      889      +38

Files with missing lines	Coverage Δ
packages/davinci-client/src/lib/davinci.utils.ts	`92.10% <ø> (-1.52%)`	⬇️
packages/davinci-client/src/lib/davinci.api.ts	`0.41% <0.00%> (-0.01%)`	⬇️
packages/davinci-client/src/lib/client.store.ts	`0.38% <0.00%> (-0.06%)`	⬇️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

github-actions · 2025-06-09T17:45:36Z

Deployed f24f072 to https://ForgeRock.github.io/ping-javascript-sdk/pr-322/f24f0727cb1b2be2c4c5041aa51bcdd0bd63c408 branch gh-pages in ForgeRock/ping-javascript-sdk

cerebrl

I'd like to suggest some pattern changes here:

I think it's best we preserve the aspect of the externalIdp method returning a "updater" function. This function is then called in the collector component like all the others. This keeps it consistent.

Within this "updater" function, we set the localStorage item. This means we only store it if a user selects the social login option. That way we don't dirty up the WebStorage if we don't have to.

This updater function can also return the URL, but it doesn't have to as the collector already has the URL on the object. Either way, we can return it as it doesn't hurt anything.

updating the api for the social login external idp method. Instead of redirecting for the application, we will return a url for the application to redirect to themselves.

cerebrl

Okay, after thinking on this for a bit, re-reviewing this PR, and thinking about long-term patterns, I'd like to suggest something a bit different holistically.

Here's a few things:

I think we should use the Storage Effect, rather than directly interact with localStorage
I'd like the location of code that triggers the write to also trigger the read from storage
I don't think davinci.api.ts should have to be aware of how to get what it needs to make the call
I'm thinking the client.store.ts should cover #2 above

Here's an example code for client.store.ts:

return {
  // ...
  externalIdp: (collector: IdpCollector): (() => string | InternalErrorResponse) => {
    const rootState: RootState = store.getState();

    const serverSlice = nodeSlice.selectors.selectServer(rootState);

    return () => {
      const error = setServerInfo(serverSlice, log);
      const redirect = collector.output.url;
      if (error) {
        return error;
      }
      return redirect;
    };
  },
  resume: async ({ continueToken }: { continueToken: string }) => {
    const serverInfo = getServerInfo(log);
    if (!serverInfo) {
      return { /* some error */ };
    }
    await store.dispatch(davinciApi.endpoints.resume.initiate({ serverInfo, continueToken }));

    const node = nodeSlice.selectSlice(store.getState());

    return node;
  },
  // ...
};

If the above doesn't make much sense, we can chat about it tomorrow.

cerebrl · 2025-06-09T20:28:24Z

packages/davinci-client/src/lib/davinci.utils.ts

 }

-export function authorize(
+export function returnRedirectUrlForSocialLogin(


I'd like to suggest we rename this function to selectIdp, since returning the URL is really secondary to the persisting the continue URL of the IdP.

cerebrl · 2025-06-09T20:33:18Z

packages/davinci-client/src/lib/davinci.utils.ts

+  logger.error(
+    'No Continue Url found, social login needs a continue url to be saved in localStorage',
+  );
+  return {
+    error: {
+      message:
+        'No Continue Url found, social login needs a continue url to be saved in localStorage',
+      type: 'network_error',
+    },
+    type: 'internal_error',
+  };


I'd like to suggest we use the pattern of handling errors first, then handling successes. You can see them in many instances throughout DaVinci Client, but good examples are client.store.ts and reducer.utils.ts here:

ping-javascript-sdk/packages/davinci-client/src/lib/node.reducer.ts

Line 182 in 9a03632

.addCase(updateCollectorValues, (state, action) => {

cerebrl · 2025-06-09T22:13:01Z

packages/davinci-client/src/lib/davinci.utils.ts

I'm not sure if this is the right location for this functionality. This utils file is supposed to be tied to the davinci.api.ts file, which this function doesn't directly relate to it. I think we need to evaluate where this logic lives.

cerebrl

Just a few small comments, and I think we are done here. Thanks for putting up with me, Ryan :)

cerebrl · 2025-06-10T20:29:12Z

packages/davinci-client/src/lib/client.store.ts

-
+  const serverStorage = createStorage<ContinueNode['server']>(
+    { storeType: 'localStorage' },
+    'socialLoginUrl',


Let's update this to reflect what's now being stored: server info.

cerebrl · 2025-06-10T20:37:02Z

packages/davinci-client/src/lib/client.store.ts

+    }): Promise<InternalErrorResponse | NodeStates> => {
+      try {
+        const serverInfo = await serverStorage.get();
+        if (serverInfo && '_links' in serverInfo) {


I get that you're narrowing here, but it feels a bit overkill for me since this very same check exists within the API module. Can we just do a cast here, and just check that truthiness of the value? We can then rely on the more strict check within the API module for the more detailed property needs.

cerebrl · 2025-06-10T20:39:06Z

packages/davinci-client/src/lib/davinci.api.ts

+        if (
+          !links ||
+          !('continue' in links) ||
+          !('href' in links['continue']) ||
+          !links['continue'].href
+        ) {


Yikes! Completely non-blocking, but it would be nice to have some sort of selector or lens to prevent this type of object path checking.

cerebrl

Let's change this name to align with what's actually stored.

cerebrl requested changes Jun 9, 2025

View reviewed changes

fix(davinci-client): return-url-from-external-idp

9a03632

updating the api for the social login external idp method. Instead of redirecting for the application, we will return a url for the application to redirect to themselves.

ryanbas21 force-pushed the fix-social-login-url-output branch from 5b5852b to 9a03632 Compare June 9, 2025 19:37

cerebrl requested changes Jun 9, 2025

View reviewed changes

cerebrl requested changes Jun 10, 2025

View reviewed changes

ryanbas21 force-pushed the fix-social-login-url-output branch from b81aec3 to b90fccd Compare June 10, 2025 21:00

chore: refactor

8f642a8

ryanbas21 force-pushed the fix-social-login-url-output branch from b90fccd to 8f642a8 Compare June 10, 2025 22:39

cerebrl requested changes Jun 10, 2025

View reviewed changes

update storage name to serverInfo

609d381

cerebrl approved these changes Jun 10, 2025

View reviewed changes

ryanbas21 merged commit 3129a9e into main Jun 11, 2025
4 checks passed

ryanbas21 mentioned this pull request Jun 11, 2025

Release PR #257

Merged

fix(davinci-client): return-url-from-external-idp #322

fix(davinci-client): return-url-from-external-idp #322

Uh oh!

Conversation

ryanbas21 commented Jun 9, 2025

JIRA Ticket

Description

Uh oh!

changeset-bot bot commented Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

nx-cloud bot commented Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cerebrl left a comment

Choose a reason for hiding this comment

Uh oh!

cerebrl left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cerebrl Jun 9, 2025

Choose a reason for hiding this comment

Uh oh!

cerebrl Jun 9, 2025

Choose a reason for hiding this comment

Uh oh!

cerebrl Jun 9, 2025

Choose a reason for hiding this comment

Uh oh!

cerebrl left a comment

Choose a reason for hiding this comment

Uh oh!

cerebrl Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

cerebrl Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

cerebrl Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

cerebrl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

changeset-bot bot commented Jun 9, 2025 •

edited

Loading

nx-cloud bot commented Jun 9, 2025 •

edited

Loading

codecov-commenter commented Jun 9, 2025 •

edited

Loading

github-actions bot commented Jun 9, 2025 •

edited

Loading

cerebrl left a comment •

edited

Loading