Make the fixed dashboard tab the default for Plus users (MNTOR-3087)

[flozia]

References:

Jira: MNTOR-3087

Description

Plus users should be taken to the ”fixed” dashboard tab by default. The only exception is when a user enters from a breach alert email, in which case, they are taken to the “action needed” tab.

TODO

• The activeTab prop in the dashboard story is set to “action-needed” for now: If this approach looks good to us we’ll need to update the dashboard story and unit test to reflect the updates in this PR.
• Update the link target to the dashboard in the breach alert email

[codemist]

@flozia As a premium user should I stay on the fixed tab by default? I'm still getting take to Action needed on refresh

Screen.Recording.2024-05-03.at.2.13.28.PM.mov

[Vinnl]

I'm not sure if I'm a fan of adding two more URLs for the dashboard, given that design is looking at consolidating the tabs into one (not sure if that was already known to you - I heard it at the work week I believe).

Even if we're adding the two URLs, I think I'd rather add them explicitly, rather than just intercepting everything past /user/dashboard/... - I think they could just import the same component? We'd also need to add redirects from the old URL to one of the new ones, to not break bookmarks and old links. And for client-side path manipulation, it's probably safer to use the router than directly manipulating window.history, to avoid breaking assumptions Next.js might be making that could lead to mismatched state.

I'd actually have expected this change to be limited to an isPremium check in the useState parameter for selectedTab / activeTab, possibly with a check for a hash fragment or query parameter to directly load a specific tab, for use by emails. Any problems with that that I'm missing?

[flozia]

As a premium user should I stay on the fixed tab by default? I'm still getting take to Action needed on refresh?

@codemist Staying on the “fixed” tab is what should happen. There was an e921dec that caused an error that might be related — is this still happening for you?

[flozia]

I'm not sure if I'm a fan of adding two more URLs for the dashboard, given that design is looking at consolidating the tabs into one (not sure if that was already known to you - I heard it at the work week I believe).

@Vinnl I was not aware of the plan to consolidate the tabs into one. I’ll try to address your comment and give a bit more context on why I arrived at this solution:

Even if we're adding the two URLs, I think I'd rather add them explicitly, rather than just intercepting everything past /user/dashboard/... - I think they could just import the same component? We'd also need to add redirects from the old URL to one of the new ones, to not break bookmarks and old links.

The idea behind using the optional catch-all segments is that this approach would not break bookmarks and old links. If no slug is provided we would redirect to the respective default route for the user.

And for client-side path manipulation, it's probably safer to use the router than directly manipulating window.history, to avoid breaking assumptions Next.js might be making that could lead to mismatched state.

Using the router leads to a re-render of the page on the server as well. Since we don’t need a full re-render the idea behind updating the URL is to only reflect the client state changes. Shallow routing is not supported by Next.js 13 as it was for Next.js 12. The recommended approach at the moment is to use the native history API for that.

I'd actually have expected this change to be limited to an isPremium check in the useState parameter for selectedTab / activeTab, possibly with a check for a hash fragment or query parameter to directly load a specific tab, for use by emails. Any problems with that that I'm missing?

Initially, I was going to use URL search params for the tabs, but I remembered a past discussion around the potential unreliabilities of using query params for navigation and was hesitant about using them. The current approach would allow to directly link to a specific tab, but could be easily switched to use searchParams, though.

[Vinnl]

Thanks for the explanation Florian, the reasoning seems solid to me.

[flozia]

Alright, @Vinnl! Since we decided to go with this approach I’ve addressed the two open to-dos:

1. Updated the link from the breach alert emails, as requested in the acceptance criteria, to always point to the “action needed” tab in 9c00730.
2. Added unit test to verify that the dashboard is being rendered with the correct active tab in 22eccd4.

[Vinnl]

Ah right, I didn't look at the description again :) Thanks for the additional changes (and opportunity to review), they look good - and there are even new tests for the older non-TS email code! Just two nits that you can take or leave.

[Vinnl]

Nit, but the test descriptions contradict each other otherwise :)

Suggested change

	it("renders the dashboard with the action needed tab selected", () => {
	const ComposedDashboard = composeStory(
	DashboardUsNoPremiumNoScanNoBreaches,
	Meta,
	);
	render(<ComposedDashboard activeTab="action-needed" />);
	const tabActionNeededTrigger = screen.getByRole("tab", {
	name: "Action needed",
	});
	expect(tabActionNeededTrigger.getAttribute("aria-selected")).toBe("true");
	});
	it("renders the dashboard with the fixed tab selected", () => {
	it("renders the dashboard with the action needed tab selected when requested", () => {
	const ComposedDashboard = composeStory(
	DashboardUsNoPremiumNoScanNoBreaches,
	Meta,
	);
	render(<ComposedDashboard activeTab="action-needed" />);
	const tabActionNeededTrigger = screen.getByRole("tab", {
	name: "Action needed",
	});
	expect(tabActionNeededTrigger.getAttribute("aria-selected")).toBe("true");
	});
	it("renders the dashboard with the fixed tab selected when requested", () => {

[flozia]

Updated in 4c80f52.

[Vinnl]

No longer marking this as TabType was surprising - is that because we the path parameter in page.tsx could be any string? The safest solution IMHO would be to have an isValidTabType user-defined type guard (essentially, extracting this if condition into a function), or otherwise at least to add a type assertion in there, rather than having the type information say that this component is fine with any string.

[flozia]

I’ve added a type assertion in 3b355f3.