fix(web-forms#883): allow users to recover from xpath errors by garethbowen · Pull Request #1908 · getodk/central-frontend

garethbowen · 2026-09-18T01:36:02Z

Closes getodk/web-forms#883

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

Manual testing, CI.

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

Discussed on the issue.

How does this change impact users? Describe intentional behavior changes from code updates. What are the regression risks?

Error messages are now shown without interrupting flow, and inline if possible.

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

No.

changeset-bot · 2026-09-18T01:36:08Z

🦋 Changeset detected

Latest commit: a826b71

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

This PR includes changesets to release 3 packages

Name	Type
@getodk/xforms-engine	Minor
@getodk/web-forms	Minor
@getodk/xpath	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

garethbowen · 2026-09-18T01:40:55Z

+							fields: references.join(', '),
+							count: references.length
+						}) }}
+					</li>


The most common case is a single error violation, potentially impacting multiple fields, so I've optimised the UX for that case.

garethbowen · 2026-09-18T02:23:27Z

@@ -91,14 +91,23 @@ describe('#format-date()', () => {

  describe('invalid dates', () => {


Now some of these throw and some return null... It may still not be right but it's closer.

garethbowen · 2026-09-18T02:37:44Z

@latin-panda This is now ready for review, but given the size the change ended up being I'm not sure we want to merge it in 1.1.1. I'm happy to leave this for 1.2.0 if that feels safer to you.

latin-panda

I haven't finished reviewing or testing, but I wanted to share my feedback so far. I'll continue tomorrow

latin-panda · 2026-09-21T18:52:28Z

  const chunks: TextChunk[] = [];
  const mediaSources: MediaSources = {};
  const chunkExpressions = getChunkExpressions(context, definition);
+  context.setError(null);


Let me see if I'm getting this correctly:

Each node/field has one error "slot". And many things write to that slot (the field's calculate, relevant, label, constraint). Each of them sets the slot to null when it succeeds.
So the label succeeds and sets it to null. But if the calculate failed before then, the error is gone. Does it make sense? 🤔
Maybe it's better to have a map of actors (calculate, constraint, etc.) and the corresponding error.

Done, thanks for spotting this!

My solution feels clunky so if you have a better way to do it I'd love to hear it, but this does work.

Thank you! The map is better, but each place still has to set and clear its own key. I haven't checked all cases but I found that createTextRange L111 sets the label error and clears it on the line below, so a broken itext shows nothing, and label, hint and messages share the same key, so one can hide the other's error.

I was having a deeper look, and each expression is already a memo that returns a Result, and result.error.message has the text we need. So we could have one memo per node that reads all the results and returns the ones that failed. Since it's reactive, it re-runs when something changes and then a fixed expression returns success, the error is removed automatically.
The createErrorValidation can read that memo instead of getError(), and the violations list stays the same.

The only thing I see so far with that approach is the calculate, setvalue and the text chunks don't return a Result yet, so they'd need that.

🤔

It's definitely a more reactive approach, and solves all the issues about keeping the error state correct. If I understand correctly, the InstanceNode would have an abstract errors accessor that child nodes would override with a memo which reactively updates based on the computation Results that it knows about. For example, the ValueNode would check for errors on the value Result, and the RepeatRangeControlled could check for errors on the computeCount Result. Each Node's accessor would probably also call the accessor on their parent node to get a combined list of errors. Is that what you were thinking?

It's another significant change so I wanted to confirm I understood what you meant before spending too much time building it out.

Yes, correct! Just one thing about call the accessor on their parent, if you mean the parent class (super.errors() + the node's own results), yes. But if you mean the parent node, the createAggregatedViolations already walks the children up to the root, so pulling from the parent would list the same error once per descendant, right?

Thinking about setvalue from my last comment, the actions run on events, so there is no Result to read. They would need to store the error (or null) of their last run on the destination node, and the node's errors() reads that 🤔

if you mean the parent class (super.errors() + the node's own results), yes

Yes this is what I meant.

Thinking about setvalue from my last comment, the actions run on events, so there is no Result to read.

Yes I think there will need to be some calculations that store errors outside of the Result. I'll see what I can come up with today.

latin-panda · 2026-09-21T19:19:48Z

@@ -339,7 +343,11 @@ const showValidationError = computed(() => {
 	if (errorBannerDismissed.value) {


I haven't tested it, but I think errorBannerDismissed should be set to false here so that new evaluations with errors display the banner.

We don't currently do that for other violations until you hit the next page or submit button, right? I feel like it would just be annoying if you have explicitly dismissed it for it to pop up again, even if now the message is different.

latin-panda

I'm still digging into the details :) Meanwhile, I wanted to share some feedback, and I replied to one of the threads with an idea for catching errors from Result more reactively. Let me know what you think!

latin-panda · 2026-09-23T20:04:33Z

-
+      case 'upload':
+        // leaf node
+        return violationReference(child);


In L38 collects attributes for the parent, right? but a leaf question never becomes context, so its attributes are skipped (the submit is not blocked on error)

Suggested change

return violationReference(child);

return [...violationReference(child), ...child.getAttributes().flatMap(violationReference)];

True! I've fixed this by moving the attributes violations into the violationReference function, so when it's getting violations for the node it also checks the attributes.

latin-panda · 2026-09-23T20:10:18Z

  readonly evaluator: EngineXPathEvaluator;
  readonly contextReference: Accessor<string>;
  readonly getActiveLanguage: Accessor<ActiveLanguage>;
+  readonly errorState: Accessor<ErrorState>;


I couldn't find anything that reads this error state. When an item label is broken (<label ref="badFn()"/>) the error seems lost?. I wonder if it could bubble up to the control's state.

latin-panda · 2026-09-23T20:13:27Z

+        message: error,
+      } as const;
+    }
+    return null;


We don't block the form if the question is relevant=false, right?
I think it should return null in that case
if !context.isRelevant() { returns null

Hmm.. In this case I'm not sure. One example is what if the expression for relevant is the thing with the error? Ultimately if this field is in an error state we can't be confident if it is relevant or not. To be on the safe side I think it's fair to block any form with an unresolved error in it.

Sounds good! If the question isn't visible and the relevant value is false, could this be used to improve the error message?
"the hidden field has invalid ....."
Or the question name/ref.

garethbowen · 2026-09-29T01:24:34Z

@latin-panda This is one of those changes that keeps going, and I'd appreciate a gut check to see if it's worth continuing. I've implemented your suggestion to make the error handling more reactive, rather than storing it in a map, and it's now reached a stable point in this PR. I have yet to handle errors for node options, labels, and hints. These are all significant additional changes so I paused to do some re-evaluation if this is the right approach.

Firstly the code is significantly more complicated after this. Just about every file in the engine now needs to have additional branches to handle errors that may be reactively returned on any update.

Secondly the engine is somewhat slower. There may be some optimisation missing but as it currently stands, the two child-vaccination smoketests run in 7.5s and 22s, up from 6.1s and 18s. So that's about a 20% slow down. It does make sense that it's somewhat slower because every change now needs to be checked to see if it's a failure, and the validation state checks many more potential sources of error.

On the other hand, a definite benefit is it will give us a clean way to handle error states that occur within the Engine itself which are currently still throwing.

Because this should be a very rare occurrence I no longer believe it's worth the performance and complexity cost.

What do you think?

If you agree, then we have a couple of options, in addition to those I outlined in the issue.

Leave the errors being thrown up to the client (status quo, close this PR). We could investigation doing some pre-render validation, and improving error messages where possible (ie: wrapping the error in another error which includes additional context like the nodeset).
Still catching the error at the engine boundary, but instead of returning a Result, set an error on the root node and return a default value. This error could be shown in the top banner but still allow for users to keep typing. The error wouldn't be shown inline but in many cases it cannot be anyway, for example, non-body elements.

latin-panda · 2026-09-29T19:31:01Z

@garethbowen, thanks for all the work on this, and for being open to trying all these ideas!

I was curious about where the slowdown comes from, so I dived into the code and ended up trying an idea on top of your branch. I didn't want to step on your code, so I put it in a separate draft PR for you to look at, no pressure to use it (the PR is not complete, it needs more test coverage and more polishing).

I found that every evaluation returns a new Success object, so the memo always thinks the value changed and everything downstream recalculates (about 4x more evaluations than master, from what I could track in my tests). The draft PR keeps your client types and the UI, and changes how the error is kept, in the expression memo, next to the value, so Result doesn't need to travel through the signatures. It also fixes a memo leak in createTextRange that is already on master. To give an idea, these are the two child vaccination smoke tests (jsdom on my machine):

test 1
- master: 2s, this branch: 3s, draft PR: 2s
test 2
- master: 5.5s, this branch: 6.5s, draft PR: 5s

What do you think?
If you agree, then we have a couple of options, in addition to those I outlined in the issue.

I agree it's not worth it with the complexity and the slowdown, but I think the draft PR removes both. It's actually close to your option 2. The difference is that the error is kept on the node and not on the root, so it clears by itself when the input is fixed. It also covers labels, hints and node options.

If you prefer to keep your approach, there is also a small change that helps a lot. The createMemo takes an equals option to decide if the result changed. The default is ===, and a new Success object is never === to the old one, so the memo always notifies. Passing a function that compares value and error.message stops that, and it runs faster. However, this still leaves the extra memos on every node and the cost of the walk that collects violations, which the draft PR reduces.

garethbowen · 2026-09-30T21:12:47Z

+    return this.getValidationViolation();
+  }
+
+  // Attributes never change once the node is built, so they are read without tracking.


In which case, shouldn't the getAttributes function also be untracked? Should attributes just be an array removing the need for the createAttributeState call at all? This is probably outside the scope of this change but could be a nice performance and simplicity win.

garethbowen · 2026-09-30T23:00:16Z

-// prettier-ignore
-type ComputedExpression<Type extends DependentExpressionResultType> = Accessor<
-	EvaluatedExpression<Type>
+export type ComputedExpression<Type extends DependentExpressionResultType> = Accessor<


This is the main difference between your draft pr and this... In the draft PR the computed expression had an error property with a function. I found this difficult to read, and meant there were two possible ways errors were being tracked, either through the error() fn or through the registerExpressionError

My latest commit uses the registerExpressionError exclusively so the ComputedExpression is unchanged from master.

garethbowen · 2026-09-30T23:04:28Z

@latin-panda Thanks! I merged your draft PR and then made a few changes...

I added back the Result for the engine evaluator. I'm more comfortable with this because then the caller is prompted to check for errors, rather than having to remember to try/catch. I didn't take it as far as with my previous version so it didn't cascade throughout the engine.
It was slightly awkward having two different ways errors could were being reported so I standardised on using the register function (comment inline).
I added a range more tests for different places expressions are executed.

This passes the tests, and executes the smoke test in about the same time as your draft PR.

What do you think?

garethbowen commented Sep 18, 2026

View reviewed changes

garethbowen requested a review from latin-panda September 18, 2026 02:36

latin-panda requested changes Sep 21, 2026

View reviewed changes

latin-panda mentioned this pull request Sep 22, 2026

The select1 with "map" appearance shows "Point saved" when no itemset options loaded getodk/web-forms#834

Open

garethbowen force-pushed the 883-node-value-result-type branch from a33c7bf to a18801b Compare September 23, 2026 00:50

garethbowen requested a review from latin-panda September 23, 2026 01:15

latin-panda requested changes Sep 23, 2026

View reviewed changes

garethbowen force-pushed the 883-node-value-result-type branch from 3307a21 to 55326b8 Compare September 23, 2026 22:55

garethbowen added 19 commits September 29, 2026 10:08

wip

a724843

more progress

6c8c970

everything works

adb1328

fix tests

9f25e79

fix some tests

48ad33a

lint

59de05e

fix tests and isSelfRelevant inside scope

e526211

make xforms-engine evaluator

29a1254

fix test helpers

dea8517

resolve todos

a4f02e5

fix test

05239c9

improve and translate error message

55e5ada

changeset

4edbfa6

lint

02752b9

revert format date to throw again

af884f9

cleanup

6c9a8f6

check for violations in groups, repeats, attributes

9c73af9

use property names to hold different errors

a7527fc

fix pagination blocking

d475771

garethbowen added 13 commits September 29, 2026 10:08

replace the actual root element rather than hardcoding /data

0f0cbe8

fix error message

28b953a

fix tests

040b6e8

don't forget about node attribute violations

ed46eef

use correct node for attribute violations

4b520a8

be more reactive

6e5c5fb

lint warnings

19dca79

fix some tests

67d9f6e

more testas

1068f27

tests

6cb72f4

fix smoke test

cfe1831

don't spam example.com

9647a29

revert change

4cf9e96

garethbowen force-pushed the 883-node-value-result-type branch from 8818fdd to 4cf9e96 Compare September 28, 2026 21:09

garethbowen added 2 commits September 29, 2026 11:07

add memo for value

9d7a39e

various cleanups and a performance improvement

3b128b6

cache errors in memo

cd615b9

garethbowen and others added 5 commits September 30, 2026 12:34

proposal: cache expression errors in the memo

67682a5

use result minimally

fbc9cb6

missed one call

36a44ae

add tests

bc856f1

more tests

0311bc2

garethbowen commented Sep 30, 2026

View reviewed changes

remove error fn on accessor

a826b71

garethbowen commented Sep 30, 2026

View reviewed changes

garethbowen requested a review from latin-panda September 30, 2026 23:00

		@@ -91,14 +91,23 @@ describe('#format-date()', () => {

		describe('invalid dates', () => {

		@@ -339,7 +343,11 @@ const showValidationError = computed(() => {
		if (errorBannerDismissed.value) {

	return violationReference(child);
	return [...violationReference(child), ...child.getAttributes().flatMap(violationReference)];

Conversation

garethbowen commented Sep 18, 2026

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 impact users? Describe intentional behavior changes from code updates. 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.

Uh oh!

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

Uh oh!

🦋 Changeset detected

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garethbowen commented Sep 18, 2026

Uh oh!

latin-panda left a comment

Choose a reason for hiding this comment

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!

latin-panda Sep 24, 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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garethbowen Sep 23, 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!

latin-panda left a comment • 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!

garethbowen commented Sep 29, 2026

Uh oh!

latin-panda commented Sep 29, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garethbowen commented Sep 30, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

changeset-bot Bot commented Sep 18, 2026 •

edited

Loading

latin-panda Sep 24, 2026 •

edited

Loading

garethbowen Sep 23, 2026 •

edited

Loading

latin-panda left a comment •

edited

Loading