Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(web): when user is opted-in redirect to the new dashboard on route change #7022

Merged
merged 3 commits into from
Nov 18, 2024

Conversation

LetItRock
Copy link
Contributor

What changed? Why was the change needed?

When the user navigates in the web app to the route that starts with /workflows and is opted-in, he will be redirected to the new Dashboard app. The same logic applies when the Web app is embedded in the new Dashboard.

Screenshots

Screen.Recording.2024-11-15.at.17.41.20.mov


export const OptInProvider = (props: PropsWithChildren) => {
const { children } = props;
const { status, isLoaded, redirectToLegacyDashboard } = useNewDashboardOptIn();

useEffect(() => {
useLayoutEffect(() => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

redirect before dom is updated

@@ -37,15 +36,13 @@ const Providers: React.FC<PropsWithChildren<{}>> = ({ children }) => {
<SegmentProvider>
<QueryClientProvider client={queryClient}>
<AuthProvider>
<OptInProvider>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the OptInProvider was only called when the application started, but we want to check opt-in when the route changes during app navigation

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering why this worked for me before but I guess it was because of this navigate() that was present before and triggered the useEffect:
https://github.com/novuhq/novu/pull/7017/files#diff-cc17c9f31a51ec7181c4bc7af9932388e5a3a796df090c2c17517d0f72401e97L23

@@ -62,6 +63,8 @@ export function PrivatePageLayout() {
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);

useOptInRedirect();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PrivatePageLayout is a top-level component that also has access to the router location, that's why the opt-in hook was placed here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue here is that PrivatePageLayout is also used in community version, the useOptInRedirect calls clerk useUser()

if (isLoaded && status === NewDashboardOptInStatusEnum.OPTED_IN) {
const currentRoute = window.location.pathname.replace('/legacy', '');
useLayoutEffect(() => {
if (shouldHandleOptInRedirect) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in short, when the new dashboard ff is on, and the user is opted-in, we want to perform the check for the current pathname on app mount, and when the route changes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the route starts with /workflows then the user will be redirected to the new dashboard app

Copy link

netlify bot commented Nov 15, 2024

Deploy Preview for novu-stg-vite-dashboard-poc ready!

Name Link
🔨 Latest commit 427c23f
🔍 Latest deploy log https://app.netlify.com/sites/novu-stg-vite-dashboard-poc/deploys/673b0c2f3e38eb0008abffa2
😎 Deploy Preview https://deploy-preview-7022--novu-stg-vite-dashboard-poc.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@ChmaraX ChmaraX merged commit 2c938e9 into next Nov 18, 2024
35 checks passed
@ChmaraX ChmaraX deleted the fix-web-opt-in-redirect branch November 18, 2024 10:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants