Skip to content

Fix #5681: improved fallback when missing faces.ajax.addOnError handler#5682

Merged
BalusC merged 6 commits into4.0from
issue_5681
Mar 20, 2026
Merged

Fix #5681: improved fallback when missing faces.ajax.addOnError handler#5682
BalusC merged 6 commits into4.0from
issue_5681

Conversation

@BalusC
Copy link
Copy Markdown
Contributor

@BalusC BalusC commented Mar 17, 2026

#5681

previously:

  • dev stage: window.alert with only half of debug info
  • not dev stage: no feedback at all

now:

  • dev stage: window.alert with full debug info, along with console.warn containing clue how to customize it, and window.onerror will be triggered
  • not dev stage: console.error with full debug info, along with console.warn containing clue how to customize it, and window.onerror will be triggered

@lprimak
Copy link
Copy Markdown

lprimak commented Mar 17, 2026

This is better, but I do not think this is enough.

not dev stage: console.error with full debug info, along with console.warn containing clue how to customize it

The problem that prompted the issue is actual users complaining. The users are on mobile phones.
In the above case, the situation would not improve. Mobile phone users do (can?) not look at the console to figure out what's going on, and are getting confused by the lack of feedback.

Thus, my suggestion is to reload the same page by default in production mode. This is when actual feedback can be given to actual users, instead of no feedback at all, which is the worst possible behavior IMHO

@BalusC
Copy link
Copy Markdown
Contributor Author

BalusC commented Mar 17, 2026

It's the developer's responsibility to implement faces.ajax.addOnError conform business requirements, not the framework's.

Note that ExceptionHandler has same issue. There's nothing in prod stage. If we change this then ExceptionHandler also needs change. And possibly many other things which only appear in dev stage. I'd rather not start this here. Also note that this dev-stage-only alert is part of the spec: https://jakarta.ee/specifications/faces/4.0/jakarta-faces-4.0#a6806

If the project stage is “development” (see Determining An Application’s Project Stage) use JavaScript “alert” to signal the error(s).

@lprimak
Copy link
Copy Markdown

lprimak commented Mar 17, 2026

It's the developer's responsibility to implement faces.ajax.addOnError conform business requirements.

Agree.

Note that ExceptionHandler has same issue.

There is a big difference here. Server-side exceptions can be seen in production logs, debugged, etc.
The reason this took so long to debug for me is that I could not figure out why the users were "hanging"
There are no logs, no error messages, nothing to determine where the errors are coming from in production.
This error is very insidious and it took me weeks to figure out what's going on.

@BalusC
Copy link
Copy Markdown
Contributor Author

BalusC commented Mar 17, 2026

The improvement in current PR should prevent/reduce that in the future.

@lprimak
Copy link
Copy Markdown

lprimak commented Mar 17, 2026

Yes, I agree it's better, but still the mobile phone case if not handled, since console errors cannot be seen on the phone.

This is especially true when testing is done in the development mode, and production is obviously in "production" mode where the issue rears its ugly head :)

@BalusC
Copy link
Copy Markdown
Contributor Author

BalusC commented Mar 17, 2026

You can decorate console.error to trigger a REST endpoint which captures and logs the message in server side. It'll solve many more mysteries than only a missing faces.ajax.addOnError.

@lprimak
Copy link
Copy Markdown

lprimak commented Mar 17, 2026

True, but it will slow down the app... especially on mobile. Not sure it's a good solution :)

@BalusC
Copy link
Copy Markdown
Contributor Author

BalusC commented Mar 17, 2026

It'll only slow down if your app spits extremely a lot of console.error lines. Even then you can still send with delay and reset delay when a new line is added. It's not impossible and definitely super helpful. You'll see :)

@lprimak
Copy link
Copy Markdown

lprimak commented Mar 17, 2026

Agreed, but I think redirect-to-self is the most sensible and simplest option :)

@BalusC
Copy link
Copy Markdown
Contributor Author

BalusC commented Mar 17, 2026

Then implement your faces.ajax.addOnError accordingly.

@lprimak
Copy link
Copy Markdown

lprimak commented Mar 17, 2026

Of course, but I am trying to argue for the most sensible default here. Not everyone has the time to debug this stuff :)

@lprimak
Copy link
Copy Markdown

lprimak commented Mar 18, 2026

Ok, at least this can be caught more easily this way

@BalusC
Copy link
Copy Markdown
Contributor Author

BalusC commented Mar 18, 2026

I added a check+trigger on window.onerror so any 3rd party monitoring tool is at least aware.

@BalusC BalusC merged commit 3fd5a2b into 4.0 Mar 20, 2026
3 checks passed
@BalusC BalusC added this to the 4.0.15 milestone Mar 20, 2026
@BalusC BalusC deleted the issue_5681 branch March 21, 2026 21:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants