Refactor `/groups` routing/file organization

[genehack]

FWIW, the client side router for all /groups URLs will make it harder to convert child components to server components in the future. (Migrating to server side components was where I stalled out on adding the groups management stuff previously...)

I don't think it was possible to set up nested dynamic routes in the Page router, but it should be possible with the new App router. Instead of using the catch all segment [[...groups]], we could switch to the simple dynamic route [group] with the file structure

static-site/app/groups/
├── [group]
│   ├── page.tsx
│   └── settings
│       ├── members
│       │   ├── page.module.css
│       │   └── page.tsx
|       ├── page.module.css
│       └── page.tsx
├── group-tiles.module.css
├── group-tiles.tsx
├── page.tsx
└── utils.ts

Originally posted by @joverlee521 in https://github.com/nextstrain/nextstrain.org/pull/1199#discussion_r2271589524

[genehack]

I completed most of the work here, before running into a problem: the dynamic error banners, (e.g., the grey box on https://next.nextstrain.org/groups/blab-private/foo) depend on the "splat" [...groups] param in order to get access to the whole requested URL, which is parsed to generate the content of the error. I couldn't figure out a way to replicate that behavior with the singular [group] capture param.

I have pushed my work in progress to a @genehack/re-org-group-routing-1201 branch, so that if somebody sees a way to support those error banners, or if there's a consensus around removing them.

[jameshadfield]

Groups URLs such as https://nextstrain.org/groups/blab/foo are 404 pages (in spirit), as they are a result of our error handling routes. As such, I have no problem with their contents being a small "This dataset doesn't exist" page, with some links of where to go next; I don't think we need to show the groups listing. If this makes it easier to shift things server-side, and that is deemed desirable, then I'd drop the listing component from such pages.

(I also didn't think that [[...foo]] routes had to be client side, but I don't claim to be a nextjs expert.)

[joverlee521]

Groups URLs such as https://nextstrain.org/groups/blab/foo are 404 pages (in spirit), as they are a result of our error handling routes.

Ah, since the [group] route is not a catch-all, this results in the top level FourOhFour page. We could add a catch-all /[group]/[...dataset] route to catch this and display the group page with the error banner. (I have it working locally, but can't push up at the moment due to Git outage).

[joverlee521]

(I also didn't think that [[...foo]] routes had to be client side, but I don't claim to be a nextjs expert.)

You're totally right. I think it currently needs use client because the router component calls useParams and uses state. This could be converted to take the params as a functional param and not store state to be a server component.

[jameshadfield]

Ah, since the [group] route is not a catch-all, this results in the top level FourOhFour page.

What, really? Our server routing hands it over to next.js, and I think it's the app/groups/[[...groups]]/page.tsx which nextjs uses. That (imports components) where the error banner comes from:

https://github.com/nextstrain/nextstrain.org/blob/1efc22ec62431c8c54b11e4df70e4d4a86daa4d0/static-site/app/groups/%5B%5B...groups%5D%5D/individual-group-page.tsx#L118-L129

[joverlee521]

Ah, since the [group] route is not a catch-all, this results in the top level FourOhFour page.

What, really? Our server routing hands it over to next.js, and I think it's the app/groups/[[...groups]]/page.tsx which nextjs uses. That (imports components) where the error banner comes from:

I meant this is the behavior when working off of JohnA's WIP branch that no longer uses the catch-all [[...groups]] route.

[joverlee521]

Two options:

1. Update to NextJS's dynamic segment route /groups/[group] (group-route branch)
2. Continue to use NextJS's optional catch-all route /groups/[[...groups]] (group-route-2 branch)

The main difference is the file organization: (1) uses nested files to conform to NextJS's routing and (2) keeps single level of files under groups/ and manages routing via our own custom GroupsPageRouter component.

[victorlin]

Thanks for the two prototypes, Jover! I find the structure of (1) more intuitive.

I did some more prototyping to figure out what else is needed to convert the pages to server components. I have something working locally based on the group-route branch, and will plan to make another issue/PR for that.

[jameshadfield]

Yeah I like (1) too, I find it easier to work things out at a glance (as long as you're vaguely familiar with next.js routing structure), e.g.:

$ tree static-site/app/groups/
static-site/app/groups/
├── [group]
│   ├── [...dataset]
│   │   └── page.tsx
│   ├── page.tsx
│   └── settings
│       ├── members
│       │   ├── page.module.css
│       │   └── page.tsx
│       ├── page.module.css
│       └── page.tsx
├── group-listing-page.tsx
├── group-tiles.module.css
├── group-tiles.tsx
├── page.tsx
├── types.ts
└── utils.ts

Note that I view this separately from which pages/components should be server-side vs client-side, but I'll write about that elsewhere.

[victorlin]

@joverlee521 would you like to make a PR from the group-route branch? I'm also happy to take this if you have other things to focus on.

[joverlee521]

@joverlee521 would you like to make a PR from the group-route branch? I'm also happy to take this if you have other things to focus on.

Thanks for pinging! This will definitely have conflicts with https://github.com/nextstrain/nextstrain.org/pull/1268. It will be easier if you can coordinate the order of both 🙏

[victorlin]

I've made some adjustments, will open a PR shortly.