New survey page based on Next.js v13 using the app router and server actions

[henrycatalinismith]

Combining all our work from #1636, #1719 and #1738. Targeting the branch for #1719 as the code changes here depend on merging the Next v13 upgrade to main. Once #1719 ships we can update the base branch of this PR to main.

[henrycatalinismith]

Nice. The playwright failures here make sense. It's the result of combining the new playwright tests we wrote at code camp with the server action version of the survey form.

[henrycatalinismith]

First little burst of activity since the hackathon produced 5459f24. The context here is I'm trying to get the survey page playwright tests green, and the executive summary is that this commit gets 2/5 of them green and then skips the remaining three. More details below.

1. ✅ submits responses
   This one was failing because it was trying to sign as the logged-in user. That's a detail we agreed to cut from the initial scope of the app router migration of this feature, so the reason it doesn't work is simply that it's not implemented. Switching to anonymous signing is fine here as a compromise because the test isn't about the signature.

2. ✅ submits email signature
   This one was failing because I had moved the survey data fetching to <SurveyForm />, which gets its API client instance for the data fetch from <ClientContext />, where it's hardcoded as BrowserApiClient. I think playwright noticed the error when the server render attempted to use the client-side API client and failed the test for it or something like that. Whatever the specifics, fetching this in the server render fixes the test and was what I originally intended to do anyway.

3. ❌ submits user signature
   Not possible to make this pass until the authentication code is migrated to the app router. Have left the test in with .skip() on it because it's not a helpful reason for the build to fail.

4. ❌ submits anonymous signature
   Actually not sure why this one is failing. Suspect there's something subtly different about the server action requests & responses. Next on the list. Skipping for now for no good reason really, just want the peace of mind of a nice green build.

5. ❌ preserves inputs on error
   The final boss. Skipping for the same reason as 4.

[henrycatalinismith]

92e0d10 gets the submits anonymous signature test enabled and passing.

My first iteration of the app router thing here was actually mega sloppy about the server component / client component boundary. Now there's a clear & coherent boundary where the <Page /> components in the src/app directory are server components that handle data fetching server side and then hand off to client components from the src/features/survey directory.

Tidying up that boundary line was enough to get this test passing.

[henrycatalinismith]

6964e23 has done two things: gotten the preserves inputs on error playwright test passing, and broken a bunch of others. The key changes here that made the test pass are as follows.

• Tweak the server action in features/surveys/actions/submit.ts so that it returns a value denoting the outcome of the submit attempt.
• Remove the submitted page and instead render the success message on the same URL as the form when the submit attempt's outcome is success. I tried to avoid this because I prefer it when people can refresh the success page. But the redirect necessary to enable it doesn't seem to fit within the new server actions philosophy.
• Install canary versions of react and react-dom in order to be able to use useFormState. Seems to be simply impossible to do error handling under this new paradigm without this hook, and it's not shipped yet.
• Update to version 14 of Next.js. Didn't love that this was a requirement, but it just doesn't work without it. You get a Converting circular structure to JSON error during yarn tsc --noEmit without it. Seems like some aspect of the canary versions of react and react-dom upsets TypeScript otherwise.

Bit of a two steps forward one step back commit given the number of new playwright test failures it introduces and the worrying dependency on canary versions of key dependencies. But! BUT! We now finally have a decently clear view here of how it'll probably look to write this kind of code at Zetkin a little while in the future once all this bleeding edge stuff goes mainstream. And god help me I think I actually like it!

[richardolsson]

I'm just having a look at the preview deployment of this, and I'm missing the "What is this question?" question that should have two options. Are multi-choice questions not implemented? Or maybe it's because there is no response_config (should default to radio buttons)?

What's being rendered on the preview

The API response for this survey

{
   "data" : {
      "access" : "open",
      "allow_anonymous" : true,
      "callers_only" : false,
      "campaign" : null,
      "elements" : [
         {
            "hidden" : false,
            "id" : 1,
            "question" : {
               "description" : "Answer whatever",
               "options" : [
                  {
                     "id" : 1,
                     "text" : "Option 1"
                  },
                  {
                     "id" : 2,
                     "text" : "Option 2"
                  }
               ],
               "question" : "What is this question?",
               "required" : false,
               "response_config" : {},
               "response_type" : "options"
            },
            "type" : "question"
         },
         {
            "hidden" : false,
            "id" : 2,
            "text_block" : {
               "content" : "With some instructions",
               "header" : "This is a text block"
            },
            "type" : "text"
         },
         {
            "hidden" : false,
            "id" : 3,
            "question" : {
               "description" : null,
               "question" : "Is this a free text question?",
               "required" : false,
               "response_config" : {
                  "multiline" : true
               },
               "response_type" : "text"
            },
            "type" : "question"
         }
      ],
      "expires" : null,
      "id" : 1,
      "info_text" : "This is a survey",
      "org_access" : "sameorg",
      "organization" : {
         "id" : 1,
         "title" : "My Organization"
      },
      "published" : "2024-02-22T06:16:19.016317",
      "signature" : "allow_anonymous",
      "title" : "A very open survey"
   }
}

[henrycatalinismith]

Good spot, I’ll get that sorted this evening. Will also have another look at the playwright thing. The canary version of react-dom is what’s behind all the failing tests so I’m keeping an eye open for new versions in case one fixes that.

[richardolsson]

The orgId in both of these places should be survey.organization.id (which may or may not be the organization that we're currently working within). You should be able to get it using useSurvey().

See #1941 for related changes (but we refrained from making changes to the URL there to avoid merge conflicts).

[henrycatalinismith]

This looks familiar!!! Did #1936 turn out to be the tip of the iceberg? I'll get this sorted right away.

[henrycatalinismith]

0265ec7

[richardolsson]

I think it's becoming time to merge this. I have some minor questions below, and I think it would be nice to merge main one last time after EOP tomorrow. Then let's get it merged fully!

[richardolsson]

When you merge main, you will find that there are lots of linting errors since we introduced a new rule in #2065, but (almost) all of them should be fixable with yarn eslint --fix.

[henrycatalinismith]

The latest commit in this branch now has a broken build. I've fixed the linter errors and removed the commented-out code but there's a new problem. The Storybook fixes have introduced a new react-dom version and it's broken the useFormState import from the canary version.

Error: src/features/surveys/components/surveyForm/SurveyForm.tsx(5,10): error TS2305: Module '"react-dom"' has no exported member 'useFormState'.

yarn why react-dom

Before the Storybook fixes

After the Storybook fixes

yarn why v1.22.21 warning ../package.json: No license field [1/4] 🤔 Why do we have the module "react-dom"...? [2/4] 🚚 Initialising dependency graph... [3/4] 🔍 Finding dependency... [4/4] 🚡 Calculating file sizes... => Found "react-dom@18.3.0-canary-763612647-20240126" info Has been hoisted to "react-dom" info This module exists because it's specified in "dependencies". info Disk size without dependencies: "13.36MB" info Disk size with unique dependencies: "13.7MB" info Disk size with transitive dependencies: "13.73MB" info Number of shared dependencies: 3

yarn why v1.22.21 warning ../package.json: No license field [1/4] 🤔 Why do we have the module "react-dom"...? [2/4] 🚚 Initialising dependency graph... [3/4] 🔍 Finding dependency... [4/4] 🚡 Calculating file sizes... => Found "react-dom@18.3.0-canary-763612647-20240126" info Has been hoisted to "react-dom" info This module exists because it's specified in "dependencies". info Disk size without dependencies: "13.36MB" info Disk size with unique dependencies: "13.7MB" info Disk size with transitive dependencies: "13.73MB" info Number of shared dependencies: 3 => Found "@storybook/addon-docs#react-dom@18.3.1" info This module exists because "@storybook#addon-essentials#@storybook#addon-docs" depends on it. info Disk size without dependencies: "4.4MB" info Disk size with unique dependencies: "4.74MB" info Disk size with transitive dependencies: "4.77MB" info Number of shared dependencies: 3

It looks like this API has also changed in React since we first worked on this branch, which is fair enough given it's marked as experimental. If you look at the docs for useActionState they say

In earlier React Canary versions, this API was part of React DOM and called useFormState.

---

Okay then, having now looked deeper into this, it seems to be a change they've shipped in React 19. So for as long as we're on 18 this doesn't affect us. This pull request has been open too long to contemplate bundling a React version upgrade into it along with everything else, I think. So I'm going to sit and get this working again with a canary version of React 18.

[henrycatalinismith]

Confirmed that the import still actually works fine and that the type error is a false alarm. I think my earlier theory about this being related to the duplicate react-dom versions was wrong. This seems like it might be related to the TypeScript upgrade instead. I made the following change.

 import { Box } from '@mui/material';
 import { FC } from 'react';
+// @ts-expect-error Erroneous `Module '"react-dom"' has no exported member 'useFormState'`
 import { useFormState } from 'react-dom';

Then tested submitting a survey, and it worked fine. The playwright tests all pass. I'm going to push this so that we have a green build here. From there we can do whatever.

[henrycatalinismith]

Finally cracked the mystery of the TypeScript error! It's a known eslint-plugin-import bug where it moves imports but leaves comments behind. Zooming out the diff in the screenshot below, you can see the removal in red at the bottom, the addition in green at the top, and the comment left behind in the old location at the bottom.

Nice to have this one tidied up. At this point I'm happy this branch is cleaned up and ready @richardolsson.

[richardolsson]

Ok, the time is finally hear! Let's get this merged and included in the next release.

Amazing work on this over the 10-or-so month lifespan of this project, @henrycatalinismith! 🙌

[henrycatalinismith]

omfg 😂