Add layout for authenticated pages #3058

flozia · 2023-05-19T14:09:28Z

Description

Adds the layout for pages that we use for authenticated pages.

Screenshot

How to test

Auth with FxA visiting /oauth/init
Visit /user/breaches and make sure the user menu shows the current users data

Checklist (Definition of Done)

Localization strings (if needed) have been added.
Commits in this PR are minimal and have descriptive commit messages.
I've added or updated the relevant sections in readme and/or code comments
I've added a unit test to test for potential regressions of this bug.
Product Owner accepted the User Story (demo of functionality completed) or waived the privilege.
All acceptance criteria are met.
Jira ticket has been updated (if needed) to match changes made during the development process.
Jira ticket has been updated (if needed) with suggestions for QA when this PR is deployed to stage.

Vinnl

Thanks for figuring this out! That said, let's not merge this yet, since it includes #3051 which is probably not the right approach. But once we've figured out auth, we can based the followup work on this.

(Alternatively, we can remove the commit from that PR from this PR?)

flozia · 2023-05-19T15:00:21Z

Thanks for figuring this out! That said, let's not merge this yet, since it includes #3051 which is probably not the right approach. But once we've figured out auth, we can based the followup work on this.

(Alternatively, we can remove the commit from that PR from this PR?)

Waiting for deciding on how to handle auth sounds good to me — marked this PR as “draft”.

pdehaan · 2023-05-19T16:46:02Z

src/app/(nextjs_migration)/(authenticated)/user/layout.tsx

+              </div>
+            </a>
+          </li>
+          <hr />


nit: Not sure this is valid. I ran the following snippet through https://validator.w3.org/nu/#textarea and it threw the following error:

Error: Element hr not allowed as child of element menu in this context. (Suppressing further errors from this subtree.)

<!DOCTYPE html> <html lang="en"> <head> <title>OK, Computer</title> </head> <body> <menu> <li>One</li> <li>Two</li> <hr /> <li>Four</li> </menu> </body> </html>

Oh. Thanks for catching this one. This right now is how we have it on the existing Monitor site — I’ll replace it because invalid syntax is really not optimal. Otherwise, the JSX for the pages is almost one-to-one taken from the existing string literals as a first step to replace the existing architecture with Next.

Seems like moving it does the trick: d98c919.

pdehaan · 2023-05-19T16:48:04Z

src/app/(nextjs_migration)/(authenticated)/user/layout.tsx

+          <li tabIndex={1}>
+            <a
+              href={AppConstants.FXA_SETTINGS_URL}
+              target="_blank"


Do we need to add rel="noreferrer noopener" to these external links, or is that handled elsewhere in headers, etc?

Note that noopener is implied when using _blank now: https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/rel/noopener - it's not harmful but not necessary in modern browsers to avoid reverse-tabnapping etc. if the FxA server was compromised.

I think we probably do want to send the referrer to FxA though.

pdehaan · 2023-05-19T16:50:05Z

src/app/(nextjs_migration)/(authenticated)/user/layout.tsx

+          className="user-menu-button"
+          title={l10n.getString("menu-button-title")}
+        >
+          <img src={fxaUser?.avatar} alt={l10n.getString("menu-button-alt")} />


Q: Should this be a vanilla <img> or Next <Image>?
Also, what's the ?. check in the src attribute? Does that imply that we might not actually have an avatar image and if so, we'd get something like an empty src="" or something like src="undefined" if somehow the fxaUser was falsey?

I guess follow up question, do we need a width= and/or height= attribute on the image (whichever version we go with) to avoid potential CLS?

Yes I have the same question re: Image, one of the benefits to switching in the ADR was to get things like the automatic image optimization without having to do the way we do it now.

If however the approach here is to first convert from existing templates -> jsx with a minimum of changes and then later convert to Next components I think that's a fine approach, but we should be explicit if so.

FWIW, looks like the only usage of <img /> tag in src/app/(nextjs_migration/** directory:

git grep -n "<img " src/app/$nextjs_migration$ | cat src/app/(nextjs_migration)/(authenticated)/user/layout.tsx:61: <img src={fxaUser?.avatar} alt={l10n.getString("menu-button-alt")} />

It's been a while since I've looked at Next.js linting, so I might need to read up on https://nextjs.org/docs/pages/building-your-application/configuring/eslint and see what the latest recommendations are.

"Well, actually…", it looks like running npx next lint flags this, fascinating!

npx next lint ./src/app/(nextjs_migration)/(authenticated)/user/layout.tsx 61:11 Warning: Using `<img>` could result in slower LCP and higher bandwidth. Consider using `<Image />` from `next/image` to automatically optimize images. This may incur additional usage or cost from your provider. See: https://nextjs.org/docs/messages/no-img-element @next/next/no-img-element ...

NOTE: And for anybody else odd enough to try getting into piping and grepping next lint output, I had to tweak the following to disable colors (since --no-color was --no-bueno, and they seem to be writing to STDERR instead of STDOUT):

FORCE_COLOR=0 npx next lint &> lint.log

Took a bit of poking around https://github.com/vercel/next.js/blob/canary/packages/next/src/cli/next-lint.ts and https://github.com/chalk/chalk to piece that together.

Thanks for the awesome grep reference!

pdehaan · 2023-05-19T16:57:34Z

src/app/(nextjs_migration)/(authenticated)/user/layout.tsx

+        <div className="pages-nav">
+          <a
+            href="/user/breaches"
+            className={`nav-item ${isBreachesPage ? "current" : ""}`}


FYI: not an endorsement one way or the other, but in FxA/SubPlat, we've been using https://www.npmjs.com/package/classnames and it seems to be a nice enhancement.

className={ classNames("nav-item", { current: isBreachesPage }) }

Same w/ L198 below, if we want to add the extra dependency (which might already be in the node_modules anyways):

className={`nav-item ${isSettingsPage ? "current" : ""}`}

I like classNames — I’ll bring it up with the other engineers.

pdehaan · 2023-05-19T17:01:54Z

src/app/(nextjs_migration)/(authenticated)/user/layout.tsx

+            href="https://relay.firefox.com/?utm_medium=mozilla-websites&utm_source=monitor&utm_campaign=&utm_content=nav-bar-global"
+            target="_blank"
+          >
+            <Image alt={l10n.getString("brand-relay")} src={RelayLogo} />


I noticed below (L237) for the Mozilla logo we added loading="lazy" attribute. Not sure if we need that here (and possibly L224 for the VPN logo).

pdehaan · 2023-05-19T17:10:50Z

src/app/(nextjs_migration)/(guest)/breaches/layout.tsx

+              {new Date(props.breach.AddedDate).toLocaleString(getLocale(), {
+                year: "numeric",
+                month: "long",
+                day: "numeric",
+              })}


I feel like the date localizer should possibly be in a helper. I think I've seen this pattern 2-3+ other places in the codebase too, so might be nice to just have a single helper that formats a date using a specified locale.

git grep -En "day: ['\"]numeric['\"]" | cat public/nextjs_migration/client/js/scan.js:118: newCard.querySelector('.exposure-scan-breach-added dd').textContent = new Date(breach.AddedDate).toLocaleString(navigator.languages, { year: 'numeric', month: 'long', day: 'numeric' }) src/app/(nextjs_migration)/(guest)/breaches/layout.tsx:100: day: "numeric", src/app/(nextjs_migration)/(guest)/breaches/page.tsx:77: <dd>{new Date(props.breach.AddedDate).toLocaleString(getLocale(), { year: 'numeric', month: 'long', day: 'numeric' })}</dd> src/client/js/partials/exposureScan.js:118: newCard.querySelector('.exposure-scan-breach-added dd').textContent = new Date(breach.AddedDate).toLocaleString(navigator.languages, { year: 'numeric', month: 'long', day: 'numeric' }) src/utils/formatDate.js:7: const options = { year: 'numeric', month: 'long', day: 'numeric' } src/views/partials/allBreaches.js:19: <dd>${new Date(breach.AddedDate).toLocaleString(getLocale(), { year: 'numeric', month: 'long', day: 'numeric' })}</dd> src/views/partials/breachDetail.js:152: breachDate: data.breach.BreachDate.toLocaleString(getLocale(), { year: 'numeric', month: 'long', day: 'numeric' }), src/views/partials/breachDetail.js:154: addedDate: data.breach.AddedDate.toLocaleString(getLocale(), { year: 'numeric', month: 'long', day: 'numeric' })

Actually, we might already have said helper function in https://github.com/mozilla/blurts-server/blob/main/src/utils/formatDate.js.

blurts-server/src/utils/formatDate.js

Lines 5 to 13 in 0bb98e8

function formatDate (date, locales) {

const jsDate = new Date(date)

const options = { year: 'numeric', month: 'long', day: 'numeric' }

const intlDateTimeFormatter = new Intl.DateTimeFormat(locales, options)

return intlDateTimeFormatter.format(jsDate)

}

export { formatDate }

pdehaan · 2023-05-19T17:14:07Z

src/app/(nextjs_migration)/(guest)/breaches/layout.tsx

+  return new Intl.ListFormat(getLocale(), {
+    type: "unit",
+    style: "short",
+  }).format(list);


pdehaan · 2023-05-19T17:18:39Z

src/app/functions/server/sessionHelpers.ts

+  cookieName: "monitor-iron-session",
+  password: process.env.IRON_SESSION_PASSPHRASE!,
+  cookieOptions: {
+    secure: process.env.NODE_ENV !== "development",


Not sure if we need some helper to determine whether we're in a specific environment. I noticed we seem to do that quite often. Where we're checking if we're in development, NOT development, heroku, NOT production, or one suspicious check against "dev" (not "development").

git grep -En "process.env.NODE_ENV \!?={2,3}" | cat next.config.js:19: `script-src 'self' ${process.env.NODE_ENV === 'development' ? "'unsafe-eval' 'unsafe-inline'" : ''} https://*.googletagmanager.com`, next.config.js:21: `connect-src 'self' ${process.env.NODE_ENV === 'development' ? 'webpack://*' : ''} https://*.google-analytics.com https://*.analytics.google.com https://*.googletagmanager.com`, next.config.js:24: `style-src 'self' ${process.env.NODE_ENV === 'development' ? "'unsafe-inline'" : ''}`, src/app/functions/server/sessionHelpers.ts:19: secure: process.env.NODE_ENV !== "development", src/appConstants.js:60:if (!process.env.SERVER_URL && process.env.NODE_ENV === 'heroku') { src/middleware/error.js:41: message: process.env.NODE_ENV !== 'production' ? errMsg : 'Something went wrong', // hide error message when in production src/middleware/error.js:42: stack: process.env.NODE_ENV === 'dev' ? err.stack : {} // hide stack when not in dev

Yeah, that dev one seems suspicious. I’ll take a look!

pdehaan · 2023-05-19T17:19:50Z

src/app/layout.tsx

+  title: "Create Next App",
+  description: "Generated by create next app",


Are these user facing at all? (ie: getting somehow shimmed into <meta> tags?)
They look a bit suspicious.

I believe these are intended for meta tags so they will show up in social media and search engines and we do indeed want to set them appropriately.

OK, think I found some useful docs on this… https://nextjs.org/docs/app/api-reference/functions/generate-metadata 👀

Yes, these are user-facing. We’ll have to port over the meta tags from the current Monitor site for which we have a separate task. :)

pdehaan · 2023-05-19T17:24:37Z

src/components/client/signinButton.tsx

-'use client'
-
-import { signIn, useSession } from "next-auth/react";
+"use client";


TIL: https://nextjs.org/docs/getting-started/react-essentials#the-use-client-directive

flozia · 2023-05-24T16:21:17Z

src/app/components/client/SiteNavigation.tsx

+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+"use client";


Ideally, this would not need to be a client-side component …

I was confused for a bit why it couldn't be (I'm still getting used to the distinction as well), but I think it's fine. We basically want this component to update whenever the user changes pages, which means we need code to run on the client-side - hence client components. Client components will still be prerendered on the server side; the differences is that additionally, code will also be shipped to the client-side to re-render it in response to user changes.

I found this comment helpful.

I see, that makes sense. Thanks for the link to the comment!

src/app/components/client/SiteNavigation.tsx

flozia · 2023-05-24T16:24:02Z

src/app/components/client/UserMenu.tsx

 };

-export const UserMenu = (props: Props) => {
+export const UserMenu = ({ session, fxaSettingsUrl }: Props) => {


We’ll have to decide how we handle duplicate components best: The one we need/would like to have for the current site and then new ones that will be built in a “React way” for Premium.

I'd suggest (like in a different comment in this PR) to keep everything that's directly copied over from the "old" site inside the (nextjs_migration) directory, and create new versions of these components outside of them. I'd like to avoid applying the global stylesheet from the old website inside the new one, and would rather incrementally recreate them using CSS modules.

So all the components in this PR that are inside /components/client would be moved, since they won't look right in the parts of the website we do "the React way".

Vinnl

Looking good and working well! I'd like to keep all the "ported" code together, but if you change that and remove the layout.tsx that I think was inadvertently added, we can merge this and start cranking out the auth'd pages!

Vinnl · 2023-05-24T16:49:10Z

src/app/components/client/SiteNavigation.tsx

+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+"use client";


I was confused for a bit why it couldn't be (I'm still getting used to the distinction as well), but I think it's fine. We basically want this component to update whenever the user changes pages, which means we need code to run on the client-side - hence client components. Client components will still be prerendered on the server side; the differences is that additionally, code will also be shipped to the client-side to re-render it in response to user changes.

I found this comment helpful.

Vinnl · 2023-05-24T16:52:58Z

src/app/(nextjs_migration)/(authenticated)/user/layout.tsx

+import { UserMenu } from "../../../components/client/UserMenu";
+import { SiteNavigation } from "../../../components/client/SiteNavigation";


Could we maybe move these components into the (nextjs_migration) folder as well? Just to indicate that we probably don't want to use them as-is on proper React pages.

I'm still not sure exactly where we'll land in the end when it comes to where to save components (i.e. do we keep them inside of /src/app/?), but that'll probably be easier to figure out when we get rid of a lot of the old folders, and moving them is easy with TypeScript anyway.

Moved them into nextjs_migration with 5a5c222.

Vinnl · 2023-05-24T16:53:58Z

src/app/(nextjs_migration)/(authenticated)/user/layout.tsx

+              href="https://support.mozilla.org/kb/firefox-monitor-faq"
+              target="_blank"
+            >
+              FAQ


TIL that we never submitted this for translation 😂

I was already wondering if “FAQ” is just something that was decided to not be translated.

Ha! Was wondering why this sounded familiar...

List breaches #2917 (comment)

Translate FAQ in v2 guestLayout and mainLayout? #2933

src/app/(nextjs_migration)/(guest)/breaches/layout.tsx

src/app/(nextjs_migration)/(guest)/layout.tsx

Vinnl · 2023-05-24T17:00:43Z

src/app/(nextjs_migration)/(guest)/page.tsx

+  const session = await getServerSession(authOptions);
+  if (session) {
+    redirect("/user/breaches");
+  }


Wasn't it intentional in the current website that we wouldn't redirect use to the dashboard from the landing page, even if they're logged in?

Right, I’ve got it the other way around with the pre-refresh site: 5f2509a and 7f99b0e.

Ah, I didn't realise you'd have to specify a redirect URL when calling signIn, but nice, that works well.

src/app/(nextjs_migration)/layout.tsx

Vinnl · 2023-05-24T17:08:12Z

src/app/components/client/UserMenu.tsx

 };

-export const UserMenu = (props: Props) => {
+export const UserMenu = ({ session, fxaSettingsUrl }: Props) => {


I'd suggest (like in a different comment in this PR) to keep everything that's directly copied over from the "old" site inside the (nextjs_migration) directory, and create new versions of these components outside of them. I'd like to avoid applying the global stylesheet from the old website inside the new one, and would rather incrementally recreate them using CSS modules.

So all the components in this PR that are inside /components/client would be moved, since they won't look right in the parts of the website we do "the React way".

…ayout file

Vinnl

Cool, nice work, let's merge this and get porting!

Vinnl and others added 5 commits May 17, 2023 16:11

Wire up Next.js to FxA using iron-session

93221d1

feat: Port main layout for authenticated pages

ecd718d

chore: Get session in layout

afbd995

fix: Provide fxa user menu with data

8c596d6

chore: Format Create Next App template

aaee7c3

flozia requested review from Vinnl and rhelmer May 19, 2023 14:09

Vinnl reviewed May 19, 2023

View reviewed changes

flozia marked this pull request as draft May 19, 2023 14:58

pdehaan reviewed May 19, 2023

View reviewed changes

flozia added 7 commits May 23, 2023 23:17

merge: nextjs -> nextjs-authenticated-layout

cea6d03

merge: Resolve conflicts

4f08a97

fix: Move hr into li element

d98c919

feat: Handle authenticated users

129679e

chore: Add todo note

14ce273

chore: Don’t use default exports for SignInButton and UserMenu

3b48eeb

chore: Move site navigation to client-side component

b595c3b

flozia commented May 24, 2023

View reviewed changes

flozia marked this pull request as ready for review May 24, 2023 16:44

Vinnl approved these changes May 24, 2023

View reviewed changes

flozia added 3 commits May 25, 2023 12:17

chore: Move components into (nextjs_migration) and remove redundant l…

5a5c222

…ayout file

chore: Remove redirect landing page -> dashboard

5f2509a

chore: Redirect to dashboard upon signin

7f99b0e

flozia requested a review from Vinnl May 25, 2023 11:08

Vinnl approved these changes May 25, 2023

View reviewed changes

flozia merged commit 0a3a7bc into nextjs May 25, 2023

flozia deleted the nextjs-authenticated-layout branch May 25, 2023 13:18

pdehaan mentioned this pull request May 25, 2023

Translate FAQ in v2 guestLayout and mainLayout? #2933

Open

	function formatDate (date, locales) {
	const jsDate = new Date(date)
	const options = { year: 'numeric', month: 'long', day: 'numeric' }
	const intlDateTimeFormatter = new Intl.DateTimeFormat(locales, options)

	return intlDateTimeFormatter.format(jsDate)
	}

	export { formatDate }

		title: "Create Next App",
		description: "Generated by create next app",

		import { UserMenu } from "../../../components/client/UserMenu";
		import { SiteNavigation } from "../../../components/client/SiteNavigation";

Add layout for authenticated pages #3058

Add layout for authenticated pages #3058

Conversation

flozia commented May 19, 2023 • edited Loading

Description

Screenshot

How to test

Checklist (Definition of Done)

Vinnl left a comment

Choose a reason for hiding this comment

flozia commented May 19, 2023

Choose a reason for hiding this comment

flozia May 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhelmer May 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vinnl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flozia May 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vinnl left a comment

Choose a reason for hiding this comment

flozia commented May 19, 2023 •

edited

Loading

flozia May 24, 2023 •

edited

Loading

rhelmer May 19, 2023 •

edited

Loading

flozia May 25, 2023 •

edited

Loading