fix(funnels): update current step based on URL change#4407
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
ilasw
requested review from
AmarTrebinjac,
capJavert,
Copilot,
nensidosari,
omBratteng,
rebelchris and
sshanzel
There was a problem hiding this comment.
Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
packages/shared/src/features/onboarding/hooks/useFunnelNavigation.ts:163
- [nitpick] Consider renaming 'stepInURL' to 'urlStepId' for enhanced clarity, as it represents the stepId extracted from the URL.
const stepInURL = searchParams.get('stepId');
There was a problem hiding this comment.
Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
packages/shared/src/features/onboarding/hooks/useFunnelNavigation.ts:163
- Ensure that 'searchParams' reliably reflects the current URL state; if this value updates asynchronously, the extracted 'urlStepId' might become stale. Consider verifying that 'searchParams' is sourced from a reactive hook.
const urlStepId = searchParams.get('stepId');
packages/shared/src/features/onboarding/hooks/useFunnelNavigation.ts:178
- [nitpick] Including 'urlStepId' in the dependency array is appropriate for reactivity; however, confirm that this does not lead to unnecessary re-renders if 'searchParams' remains stable across renders.
[funnel.id, urlStepId]
| () => { | ||
| // Check if the URL has a stepId parameter or if there is a session | ||
| const stepId = searchParams.get('stepId') ?? session.currentStep; | ||
| const stepId = urlStepId ?? session.currentStep; |
There was a problem hiding this comment.
would it not be better, if session.currentStep is different from stepid, then redirect to that /?step=session.currentStep
if not just have the step state in URL always?
There was a problem hiding this comment.
The idea is that the URL takes priority over currentStep from the session, since users can navigate through the steps.
I’d prefer not to introduce redirect logic here, as it’s very easy to end up in a loop with this kind of setup.
There was a problem hiding this comment.
Could this resolving be server side logic instead of useEffect, and then client part just gets the stepId in the atom? But leaving up to you guys since you are more familiar with system.
There was a problem hiding this comment.
Great idea! I've moved this logic in the SSR ✔️
There was a problem hiding this comment.
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
packages/webapp/pages/helloworld/index.tsx:79
- Using a template literal for query?.stepId will yield the string 'undefined' when query.stepId is missing, causing unintended behavior. Consider replacing it with a nullish coalescing operator, for example: query?.stepId ?? boot.data?.funnelState?.session?.currentStep.
const initialStepId: string | null = `${query?.stepId}` || boot.data?.funnelState?.session?.currentStep;
| position, | ||
| skip, | ||
| step, | ||
| isReady: isInitialized.current, |
There was a problem hiding this comment.
[nitpick] Using a ref to manage the isReady flag makes it non-reactive, which might lead to components not re-rendering when initialization completes. Consider using useState instead to ensure that changes to isReady trigger a re-render in consuming components.
There was a problem hiding this comment.
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
packages/shared/src/features/onboarding/hooks/useFunnelNavigation.ts:88
- [nitpick] Consider renaming 'isInitialized' to 'hasInitialized' for improved clarity, as it indicates whether initialization has occurred.
const isInitialized = useRef<boolean>(false);
packages/shared/src/features/onboarding/hooks/useFunnelNavigation.ts:182
- The useEffect handling initial navigation includes 'step.id' in its dependency array, which could lead to unintended re-runs if 'step.id' changes after the initial render. Consider revising the dependency array to ensure the effect only runs on mount as intended.
if (initialStepId) {
Changes
Listen also stepId on URL in order to update current step on back navigation
Events
Did you introduce any new tracking events?
Experiment
Did you introduce any new experiments?
Manual Testing
Caution
Please make sure existing components are not breaking/affected by this PR
Preview domain
https://fix-back-history.preview.app.daily.dev