Skip to content

Commit

Permalink
fix: BYOS redundant onScreenUpdate calls RELEASE (#1031)
Browse files Browse the repository at this point in the history
**Background:**
Open-Web reported that the `onScreenUpdate` callback is being called
more often than expected
It should only be triggered after state changes and `next` calls, but
they noticed it’s also firing in other scenarios

**Root Cause:**
After investigating, I found that this happens because we modified the
flow state behavior to trigger callbacks on every call to the update
function, even when there’s no actual state change
As a result, `onScreenUpdate` is called on any flow state update, even
when it’s not needed

**For example:**
We update the state when the application’s visibility changes, and this
causes unnecessary `onScreenUpdate` calls

**The Fix:**
I made the following changes to address this:
• Reverted the flow state behavior: Callbacks will now only be triggered
when there’s an actual state change, not on every update call
• Reverted the step state behavior: Callbacks will now only be triggered
when there’s an actual state change, not on every update call
• Added a next call timestamp: This acts as a cache buster so that
`onScreenUpdate` still gets triggered when next is called, even if the
state hasn’t changed
• Updated the polling mechanism: The polling mechanism started going out
of sync because of the above change, so I adjusted it accordingly

**Verification & Testing:**
I validated these changes with several flows, and everything seems to be
running without issues
However, I’m not familiar with every use case of the SDK, so I’d really
appreciate your help here

I also noticed that unit tests alone aren’t enough to catch all
potential regressions
I encountered some issues only when running actual flows, even though
all unit tests passed
I’ve added more unit tests to cover these cases, but we still need some
manual testing to be sure

**Request:**
Could you please review this change and run some basic tests in the
areas you’re familiar with?
This will help us ensure there are no regressions and that we can
release this fix with greater confidence

Thanks in advance for your cooperation!
  • Loading branch information
nirgur authored Feb 25, 2025
1 parent 350c900 commit bac7a25
Show file tree
Hide file tree
Showing 4 changed files with 129 additions and 24 deletions.
10 changes: 6 additions & 4 deletions packages/sdks/web-component/src/app/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -84,9 +84,11 @@
descopeEle.onScreenUpdate = async (screenName, context, next, ref) => {
console.log('onScreenUpdate', screenName, context);

// if (screenName !== 'Sign In') return false;

// ref.innerHTML = `
// <form>
// <input type="text" name="email" placeholder="Email" />
// <input type="text" name="password" placeholder="Password" />
// <button type="submit">Submit</button>
// </form>
// `;
Expand All @@ -96,10 +98,10 @@
// const formData = new FormData(e.target);
// const data = Object.fromEntries(formData.entries());

// next('Ppb_65tyyn', data);
// });
// next('pXVwWREG7M', data);
// }, { once: true });

return false;
// return true;
};
</script>
</body>
Expand Down
44 changes: 25 additions & 19 deletions packages/sdks/web-component/src/lib/descope-wc/DescopeWc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -76,9 +76,7 @@ class DescopeWc extends BaseDescopeWc {

flowState: State<FlowState>;

stepState = new State<StepState>({} as StepState, {
forceUpdate: true,
});
stepState = new State<StepState>({} as StepState);

#pollingTimeout: NodeJS.Timeout;

Expand Down Expand Up @@ -386,9 +384,6 @@ class DescopeWc extends BaseDescopeWc {
}

this.#toggleScreenVisibility(isCustomScreen);
// in order to call onScreenUpdate after every next call
// and not only when there is a state change, we need to force update when we are rendering a custom screen
this.flowState.forceUpdate = isCustomScreen;
}

async onFlowChange(
Expand Down Expand Up @@ -423,6 +418,7 @@ class DescopeWc extends BaseDescopeWc {
samlIdpResponseRelayState,
nativeResponseType,
nativePayload,
reqTimestamp,
...ssoQueryParams
} = currentState;

Expand Down Expand Up @@ -682,12 +678,14 @@ class DescopeWc extends BaseDescopeWc {
return;
}

this.#handlePollingResponse(
executionId,
stepId,
flowConfig.version,
projectConfig.componentsVersion,
);
if (isChanged('action')) {
this.#handlePollingResponse(
executionId,
stepId,
flowConfig.version,
projectConfig.componentsVersion,
);
}

// if there is no screen id (possibly due to page refresh or no screen flow) we should get it from the server
if (!screenId && !startScreenId) {
Expand Down Expand Up @@ -791,10 +789,7 @@ class DescopeWc extends BaseDescopeWc {
};
}

this.loggerWrapper.debug(
'Going to render screen with id',
stepStateUpdate.screenId,
);
this.loggerWrapper.debug('Got a screen with id', stepStateUpdate.screenId);

await this.#handleCustomScreen(stepStateUpdate);

Expand Down Expand Up @@ -891,14 +886,16 @@ class DescopeWc extends BaseDescopeWc {
);
}

this.#handleSdkResponse(sdkResp);
// will poll again if needed
// handleSdkResponse will clear the timeout if the response action is not polling response
this.#handlePollingResponse(
executionId,
stepId,
flowVersion,
componentsVersion,
);

this.#handleSdkResponse(sdkResp);
}, delay);
}
};
Expand Down Expand Up @@ -957,7 +954,11 @@ class DescopeWc extends BaseDescopeWc {
this.loggerWrapper.error(errorText);
}

const { status, authInfo, lastAuth } = sdkResp.data;
const { status, authInfo, lastAuth, action } = sdkResp.data;

if (action !== RESPONSE_ACTIONS.poll) {
this.#resetPollingTimeout();
}

if (status === 'completed') {
if (this.storeLastAuthenticatedUser) {
Expand All @@ -971,7 +972,6 @@ class DescopeWc extends BaseDescopeWc {
executionId,
stepId,
stepName,
action,
screen,
redirect,
openInNewTabUrl,
Expand All @@ -981,10 +981,15 @@ class DescopeWc extends BaseDescopeWc {
nativeResponse,
} = sdkResp.data;

// this is used as a cache buster
// we want to make sure the onScreenUpdate will be called after every next call even if the state was not changed
const reqTimestamp = Date.now();

if (action === RESPONSE_ACTIONS.poll) {
// We only update action because the polling response action does not return extra information
this.flowState.update({
action,
reqTimestamp,
});
return;
}
Expand Down Expand Up @@ -1022,6 +1027,7 @@ class DescopeWc extends BaseDescopeWc {
samlIdpResponseRelayState: samlIdpResponse?.relayState,
nativeResponseType: nativeResponse?.type,
nativePayload: nativeResponse?.payload,
reqTimestamp,
});
};

Expand Down
1 change: 1 addition & 0 deletions packages/sdks/web-component/src/lib/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ export type FlowState = {
samlIdpResponseRelayState: string;
nativeResponseType: string;
nativePayload: Record<string, any>;
reqTimestamp: number;
} & SSOQueryParams;

export type StepState = {
Expand Down
98 changes: 97 additions & 1 deletion packages/sdks/web-component/test/descope-wc.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2285,6 +2285,49 @@ describe('web-component', () => {
});
});

it(
'should not have concurrent polling calls',
async () => {
startMock.mockReturnValueOnce(generateSdkResponse());

const MIN_NUM_OF_RUNS = 15;

let isRunning = false;
let counter = 0;
let isConcurrentPolling = false;

nextMock.mockImplementation(
() =>
new Promise((resolve) => {
if (isRunning) {
isConcurrentPolling = true;
}
counter += 1;
isRunning = true;
setTimeout(() => {
resolve(
generateSdkResponse({
action: RESPONSE_ACTIONS.poll,
}),
);

isRunning = false;
}, 100);
}),
);

pageContent = '<div data-type="polling">...</div><span>It works!</span>';
document.body.innerHTML = `<h1>Custom element test</h1> <descope-wc flow-id="otpSignInEmail" project-id="1"></descope-wc>`;

await waitFor(() => expect(counter).toBeGreaterThan(MIN_NUM_OF_RUNS), {
timeout: WAIT_TIMEOUT * 5,
});

if (isConcurrentPolling) throw new Error('Concurrent polling detected');
},
WAIT_TIMEOUT * 5,
);

describe('native', () => {
it('Should prepare a callback for a native bridge response and broadcast an event when receiving a nativeBridge action', async () => {
startMock.mockReturnValueOnce(
Expand Down Expand Up @@ -5316,6 +5359,49 @@ describe('web-component', () => {
timeout: 20000,
},
);

await waitFor(
() =>
expect(
descopeWc.shadowRoot.querySelector('#content-root'),
).not.toHaveClass('hidden'),
{
timeout: 20000,
},
);
});
it('should render a flow screen when onScreenUpdate is not set', async () => {
startMock.mockReturnValue(generateSdkResponse());

pageContent = `<div>Loaded123</div><descope-link class="descope-link" href="{{user.name}}">ho!</descope-link>`;

document.body.innerHTML = `<h1>Custom element test</h1> <descope-wc flow-id="otpSignInEmail" project-id="1"><button>Custom Button</button></descope-wc>`;

const descopeWc = document.querySelector('descope-wc');

await waitFor(() => screen.getByShadowText('Loaded123'), {
timeout: WAIT_TIMEOUT,
});

await waitFor(
() =>
expect(descopeWc.shadowRoot.querySelector('slot')).toHaveClass(
'hidden',
),
{
timeout: 20000,
},
);

await waitFor(
() =>
expect(
descopeWc.shadowRoot.querySelector('#content-root'),
).not.toHaveClass('hidden'),
{
timeout: 20000,
},
);
});
it('should render a custom screen when onScreenUpdate returns true', async () => {
startMock.mockReturnValue(generateSdkResponse());
Expand Down Expand Up @@ -5349,8 +5435,18 @@ describe('web-component', () => {
timeout: 20000,
},
);

await waitFor(
() =>
expect(
descopeWc.shadowRoot.querySelector('#content-root'),
).toHaveClass('hidden'),
{
timeout: 20000,
},
);
});
it('should call onScreenUpdate when rendering a custom screen even if there is no state change', async () => {
it('should call onScreenUpdate after "next" call, even if there is no state change', async () => {
startMock.mockReturnValue(generateSdkResponse());

nextMock.mockReturnValue(generateSdkResponse());
Expand Down

0 comments on commit bac7a25

Please sign in to comment.