Separate forms app by garethbowen · Pull Request #1608 · getodk/central-frontend

garethbowen · 2026-06-08T06:06:07Z

Things left to do...

The asset bundles are still combined so users of either app end up downloading all the code for both. I think I have a way forward but this doesn't impact the majority of this review so I thought it was worth a first look
The unit tests are being difficult, because they rely on a lot of the infrastructure of central to run. I got some running on vitest but the solution might be to go back to karma and do the bare minimum.
Depends on associated central patch: Separate forms app central#1960

getodk/web-forms#535

What has been done to verify that this works as intended?

Why is this the best possible solution? Were any other approaches considered?

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Does this change require updates to user documentation? If so, please file an issue here and include the link below.

Before submitting this PR, please make sure you have:

run npm run test and npm run lint and confirmed all checks still pass OR confirm CircleCI build passes
verified that any code or assets from external sources are properly credited in comments or that everything is internally sourced
run npm run changeset to generate a changeset file for changes that should be included in the release notes

work in progress - do not merge

changeset-bot · 2026-06-08T06:06:12Z

🦋 Changeset detected

Latest commit: 10d4efc

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
@getodk/central-frontend	Minor
@getodk/forms	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

latin-panda

It's great to see this coming along! I did an initial review, and tomorrow I'll take another deep look.

latin-panda · 2026-06-08T17:57:46Z

+  configureServer(server) {
+    server.middlewares.use((req, res, next) => {
+      // must match the regex paths defined in Nginx
+      const newSubmissionUrlRegex = /^\/projects\/\d+\/forms\/[^/]+(?:\/draft)?(?:\/preview|\/submissions\/new(?:\/offline)?\/?|\/submissions\/[^/]+\/edit)\/?/;


Same feedback from here

Is there a way to define this in one place and share it in the different configs?

I don't know if that'll be possible between nginx and vue, but I'm sure we can at least get the nginx using the same config. There's an ongoing convo about this because the mismatch between dev and prod is risky. I think the docker work will help us move in that direction.

I've added comments and renamed the regexes to be more clear.

latin-panda · 2026-06-08T17:59:19Z

-  instanceId: String
-});
-
 const emit = defineEmits(['loaded']);


Who calls this emit? Is it dead code?

Yes! Thank you.

latin-panda · 2026-06-08T18:11:37Z

+  const { status, statusText, headers } = response;
+
+  const fetchHeaders = new Headers();
+  for (const [key, value] of Object.entries(headers)) {


The response comes from the native fetch, so the headers are a Headers type that should be iterated using its entries function.

Suggested change

for (const [key, value] of Object.entries(headers)) {

for (const [key, value] of headers.entries()) {

latin-panda · 2026-06-08T18:17:08Z

+  router.beforeEach(to => {
+    if (to.fullPath.startsWith('/#/')) {
+      // This must do a full page refresh because the page may end up in a different vue app
+      window.location.href = to.fullPath.substring(2);


The user's back button takes them to the hash URL, which redirects again. The replace swaps the entry.
Also, it can return false here to cancel any forward loading in central (if any callback is reacting to navigation events)

Suggested change

window.location.href = to.fullPath.substring(2);

window.location.replace(to.fullPath.slice(2));

return false;

Nice. Thanks.

It turns out returning false cancels the request so which caused playwright to bail. Because this isn't a security issue I've left it continuing on which might be a little bit of a waste but means the request completes cleanly.

latin-panda · 2026-06-08T18:22:18Z


-#web-form-renderer-submission-modal pre {
-  white-space: pre-wrap;
+if (isEdit.value) {


Preferably wrap in an init function or onMount hook

latin-panda · 2026-06-08T18:31:46Z

@@ -0,0 +1,32 @@
+<!DOCTYPE html>
+<!--
+Copyright 2017 ODK Central Developers


There have been conversations in Slack about this (maybe in the team channel?). I think we resolved to drop this for new development, since this is a new app. It could be dropped.

Did you decide to leave it here and remove it from the other files? Or perhaps it has not been pushed yet? 😅

Ok! Removed that one too.

garethbowen · 2026-06-09T03:24:06Z

@latin-panda I've resolved all your comments. I've also...

fixed the bundling so the central app no longer downloads web-forms
added error handling so if your session times out you get redirected to the login page

latin-panda

I haven't finished yet, but I'm submitting the first part of today's review

latin-panda · 2026-06-09T07:19:15Z

+  let body;
+  const contentType = headers.get('content-type');
+  if (contentType && (contentType.includes('application/json') || contentType.includes('application/geo+json'))) {
+    body = await response.json();


In L254, this body is passed to the Response constructor, but it doesn't accept the data type that .json() produces and ends up converting it to a string ("[object Object]"). That's probably why it initially used JSON.stringify(body).

Oh wow... looking at this now I don't think we need this function at all. It was used to transform an axios response into a native response, but because it's all native that's not necessary.

Removed!

latin-panda · 2026-06-09T07:21:14Z

+    url = `/v1/projects/${props.form.projectId}/forms/${props.form.xmlFormId}/submissions/${props.instanceId}`;
+    method = 'PUT';
+  } else {
+    url = `/v1/projects/${props.form.projectId}/forms/${props.form.xmlFormId}${draftPath}/submissions`;


When does it append a st query parameter for public-link?

Same question for L59, L151, L262, 268, 279.

What do you think about having a URL builder and unit testing it? Looking to centralize it

As far as I can tell it's only used for fetching the formXml which is handled in the api. After that the st isn't needed.

Regarding centralising it, I think that's a good idea. At the moment these are only used once each so I just inlined them.

latin-panda · 2026-06-09T07:30:21Z

-    clearFormCallback({ next: POST_SUBMIT__NEW_INSTANCE });
-  };
+const submissionPath = () => {
+  return `/v1/projects/${props.form.projectId}/forms/${props.form.xmlFormId}/submissions/${props.instanceId}`;


The suffix is used only for api calls, for L116 we need it without /v1/

Good catch! Amazing that it worked at all... removed now.

latin-panda · 2026-06-09T07:34:37Z

+    if (response.ok) {
+      const data = await response.json();
+      result = { success: true, data };
+    }


It should assign result when response.ok is false to ensure consistent handling in subsequent flows after this code.

latin-panda · 2026-06-09T07:41:06Z

-  submissionModal.hide();
+const getAttachment = (requestUrl: URL) => {
+  const encodedName = encodeURIComponent(requestUrl.pathname.split('/').pop()!);
+  const draftPath = '';


In L64, there's a check for props.form.draft that we could use here as well.

Or does it even need draftPath? What happens if there are attachments for form drafts?

You're right! It is expected by the API. Fixed.

latin-panda · 2026-06-09T08:08:54Z

+  } catch (e) {
+    if (e instanceof RequestError && e.statusCode >= 401 && e.statusCode < 404) {
+      // not logged in
+      window.location.href = '/login?next=/wf' + window.location.pathname;


I think it needs to preserve window.location.search for the navigation after login

Nice. Done.

latin-panda · 2026-06-09T08:11:54Z

+    xform.value = await getFormXml(formConfig.projectId, formConfig.xmlFormId, formConfig.draft, st)
+    webFormsEnabled.value = true;
+  } else {
+    if (offline) {


Little comment to not lose track.

Suggested change

if (offline) {

if (offline) {

// TODO: Update once Web Forms has support for offline

latin-panda · 2026-06-09T08:24:36Z

+  <template v-else-if="webFormsEnabled">
+    <WebFormRenderer :form="form!" :xform="xform!" :instance-id="instanceId" :action-type="props.actionType ?? 'new'"/>
+  </template>
+  <template v-else>
+    <EnketoIframe :form="form!" :enketo-id="enketoId" :action-type="props.actionType ?? 'new'"/>
+  </template>


It doesn't seem right to default to new here. Instead of silently prompting the user to create a new submission, it would be better to display an error message and an option to close the page. For example, if editing isn't supported by Enketo, it could show: Editing is only supported for Web Forms. Please contact the administrator.

I think defaulting to 'new' is correct in the case when no action type is provided.

However you're right that we should catch the case where they're editing in Enketo. I've caught that out and given a non-specific error above (404). A specific error is new functionality - I'm happy to raise an issue if you think it's worth that.

latin-panda · 2026-06-09T08:42:37Z

+    "vue-i18n": "^10.0.7",
+    "vue-router": "^4.6.3"
+  },
+  "devDependencies": {


Is there a way we can declare primevue and the local packages (web-forms, xforms-engine) as direct dependencies here? It helps a lot with readability so we know exactly what the app consumes.

Maybe not... 🤔
Leaving the comment open just in case there's an idea

I've added primevue because that one we should definitely have.

I'm unsure whether we should include monorepo packages in the dependencies...

latin-panda · 2026-06-09T08:56:12Z

+  const url = `/v1/projects/${projectId}/forms/${formId}${draftPath}.xml${qs}`;
+  const response = await fetch(url);
+  if (!response.ok) {
+    const result = await response.json();


If response.json() breaks (response.ok is falsy, maybe corrupted), is there any other catch to handle it nicely?

I don't think so. We could catch it here and wrap it up with a 500 error or something? But I think letting it go and be handled by the caller is ok too.

latin-panda

This is the second part of today's review.
Mainly small safe feedback that improves readability

latin-panda · 2026-06-09T18:25:26Z

Suggested change

if (returnUrlPascalCase && typeof returnUrlPascalCase === 'string') {

return returnUrlPascalCase;

}

if (returnUrl && typeof returnUrl === 'string') {

return returnUrl;

}

return null;

latin-panda · 2026-06-09T18:30:41Z

+<script setup lang="ts">
+import { ref, defineAsyncComponent } from 'vue';
+import { useRoute, useRouter } from 'vue-router';
+import { type Form, getFormByEnketoId, getFormByFormId, getFormXml, getProject, type Project, queryString, RequestError } from '../utils/api.ts';


Is lint or prettify running? This is very long. It should have been caught and fixed automatically

At the moment it's using central's config for eslint, and no prettify at all. As most of the code came from central I took the path of least resistance. I've manually broken the line here.

I'm looking forward to getting web-forms, forms, and central on the same setting.

latin-panda · 2026-06-09T18:39:40Z

-});
+if (inst) {
+  inst.appContext.app.use(PrimeVue, { theme: { preset: odkThemePreset, options: { darkModeSelector: false } } });
+  // TODO this should work instead of the above, but haven't got it working yet...


Is this planned for this round or for a follow-up PR?

At the moment it's working so I'm ok leaving it in. If you have any ideas please let me know!

latin-panda · 2026-06-09T18:41:51Z

+const handleSubmit = async (
+  payload: MonolithicInstancePayload,
+	clearFormCallback: Function
+) => {


The spacing looks off to me

Suggested change

const handleSubmit = async (

payload: MonolithicInstancePayload,

clearFormCallback: Function

) => {

const handleSubmit = async (

payload: MonolithicInstancePayload,

clearFormCallback: Function

) => {

latin-panda · 2026-06-09T18:42:59Z

+  if (isEdit.value) {
+    fetchSubmissionAttachments().then(() => {
+      loading.value = false;
+    })


I think lint catches these too

Suggested change

})

});

Linting is a mess... I'm using the old central eslint config but now because I'm using typescript it's not happy. I'm going to look at how hard it would be to conform with web-forms style...

Ok I've switched to using the same config as web-forms packages...

latin-panda · 2026-06-09T18:51:39Z

+export const i18n = createI18n({
+  locale: fallbackLocale,
+  fallbackLocale,
+  messages: { },


Is it correct that messages is empty?

All the messages required are inline in the vue files. This will change if/when we modernise the i18n.

latin-panda · 2026-06-09T18:53:34Z

+import { i18n } from './i18n'
+
+const app = createApp(Forms as Component);
+app.use(webFormsPlugin);


Why defining Primevue in web-form-renderer.vue L35 instead of here main.ts? 🤔

Because primevue isn't needed for enketo rendering so I wanted to leave it out if possible.

latin-panda · 2026-06-09T18:56:31Z

@@ -0,0 +1,32 @@
+<!DOCTYPE html>
+<!--
+Copyright 2017 ODK Central Developers


Did you decide to leave it here and remove it from the other files? Or perhaps it has not been pushed yet? 😅

latin-panda · 2026-06-09T18:58:25Z

+    "vue-i18n": "^10.0.7",
+    "vue-router": "^4.6.3"
+  },
+  "devDependencies": {


Maybe not... 🤔
Leaving the comment open just in case there's an idea

latin-panda · 2026-06-09T19:24:07Z

+    case 'forms.js':       return size > 2_000_000;
+    case 'web-form.js':    return size > 2_000_000; // Matches web-form-renderer.js


Is this L63 the web forms plugin or the web-forms renderer of app/forms? If the latter, it can be lowered. If the former, we need to update the comment.

When running the script:
//:build:web: dist/assets/web-form-renderer-CycNBHBr.js 985.19 kB │ gzip: 291.12 kB
//:build:web: dist/forms/forms-fTznoukg.js 1,848.76 kB │ gzip: 779.64 kB

Good catch. I reduced the size to 1MB.

Funny side note: The reason it matches web-form-renderer.js is because the word "renderer" is 8 letters long which exactly matches the regex which is designed to strip off the hash suffix.

garethbowen · 2026-06-10T06:03:46Z

@latin-panda Thank you for your continued reviews! I've responded to your comments, and this is good to go again.

garethbowen added 30 commits June 8, 2026 15:34

feat: create a separate app for form rendering

9db8ac8

work in progress - do not merge

clean up types

68ccdc4

get submission attachments working

7bb051e

get enketo iframe loading

e59fe82

digging out the tendrils of forms rendering

a0f262b

get unit tests passing

de1d467

linting

144df86

get editing working

b7499df

get first e2e test passing

5441c15

add forms app to nginx routing

9dbe21d

reference custom branch of central for testing nginx.conf

9d39d3a

roll up not down

a794d82

assets names have changed

cace97c

allow for punctuation in xml ids

1567452

some tests passing

5a6baaa

more tests

94dc714

full page refresh for /#/ urls

8165cef

edit subscription

da5d62d

fix error handling test

3ccc9c2

lint

06209fb

don't test # redirect

da1d467

🤦

937ce89

get drafts working

6970881

more tests green

dbdea81

primevue modals and buttons, and translations

8296c84

linting and fixed translation

fb65fcc

need to figure this out another day

9ffbc06

either close button will do!

b8e2db8

cleanup

41fbc42

transifixed

4fc0e2d

garethbowen added 2 commits June 8, 2026 16:29

basic error handling

2c285ca

simple loading screen

055b4c6

garethbowen requested a review from latin-panda June 8, 2026 06:06

latin-panda requested changes Jun 8, 2026

View reviewed changes

garethbowen added 9 commits June 9, 2026 10:36

remove web-forms version from central

a3929ac

separate js and css bundling

ed206f6

remove unused file

4f8764e

fix check bundle size

7f93896

review feedback

624c67e

update tests for location.replace

83b9cce

returning false cancels the naviation and breaks playwright

0847ca2

handle errors gracefully, especially on session timeout

a10fc45

added changeset

0568083

garethbowen requested review from latin-panda and sadiqkhoja June 9, 2026 03:24

latin-panda requested changes Jun 9, 2026

View reviewed changes

garethbowen added 6 commits June 10, 2026 12:14

several test improvements, still failing on primevue

df289e6

review feedback

8573e33

use chrome headless

b4af0e8

switch over to web-forms linting

efc901e

get forms tests working in ci

559261e

fix a couple of missed computation changes

429ef6d

garethbowen requested a review from latin-panda June 10, 2026 04:18

garethbowen marked this pull request as ready for review June 10, 2026 04:18

fix progress spinner

d727139

fix bundle size limit

10d4efc

	for (const [key, value] of Object.entries(headers)) {
	for (const [key, value] of headers.entries()) {

	window.location.href = to.fullPath.substring(2);
	window.location.replace(to.fullPath.slice(2));
	return false;

	if (offline) {
	if (offline) {
	// TODO: Update once Web Forms has support for offline

+ if (returnUrlPascalCase && typeof returnUrlPascalCase === 'string') {
+   return returnUrlPascalCase;
+ }
+ if (returnUrl && typeof returnUrl === 'string') {
+   return returnUrl;
+ }
+ return null;

		case 'forms.js': return size > 2_000_000;
		case 'web-form.js': return size > 2_000_000; // Matches web-form-renderer.js

Conversation

garethbowen commented Jun 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What has been done to verify that this works as intended?

Why is this the best possible solution? Were any other approaches considered?

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Does this change require updates to user documentation? If so, please file an issue here and include the link below.

Before submitting this PR, please make sure you have:

Uh oh!

changeset-bot Bot commented Jun 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

latin-panda left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garethbowen commented Jun 9, 2026

Uh oh!

latin-panda left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garethbowen Jun 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

garethbowen commented Jun 8, 2026 •

edited

Loading

changeset-bot Bot commented Jun 8, 2026 •

edited

Loading

garethbowen Jun 10, 2026 •

edited

Loading

latin-panda left a comment •

edited

Loading