Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show messages for users if the ROOT_URL is wrong, show JavaScript errors #18971

Merged
merged 15 commits into from
Mar 30, 2022

Conversation

wxiaoguang
Copy link
Contributor

@wxiaoguang wxiaoguang commented Mar 2, 2022

ROOT_URL issues

Many maintainers have answered the issues about incorrect ROOT_URL again and again.

Using HTMLURL(absolute URL) in templates is the root case that Gitea can only serve web under ROOT_URL. That's why we have to force users to set ROOT_URL correctly, and that's why this PR came. If all URLs in template can be refactored to use Relative URL correctly, then we do not need to force users set ROOT_URL correctly again and again.

Actually, at the moment, there are 2 major cases:

  • The assets can not be loaded (AppSubUrl != "" and users try to access http://host:3000/)
  • The ROOT_URL is wrong, then many URLs in Gitea are broken.

This PR can detect these problems, and show enough information to users. No i18n is needed because these messages should only be processed by Gitea admin.

JavaScript error issues

And, there are many users affected by JavaScript errors:

And more and more

If these problems (JS errors) can be found at first time, then maintainers do not need to ask about how bug occurs again and again.

Screenshots:

Can not load index.js

image

Incorrect ROOT_URL

image

JavaScript error

mainly caused by incorrect customized templates, or incorrect cached files

image

window.config broken

image

image

FAQ

What if some users do not want to see these errors?

There is a specialized CSS class "js-global-error" for the error message, then end users still have a chance to hide these error messages by customized CSS styles. But please DO NOT do so, otherwise if the users ignore the warning, there will be a lot of UI problem on their side, for example: the issue preview / inline PR comment don't work, etc.

What if I see "Your ROOT_URL in app.ini is undefined"

There must be something wrong with your JS assets or templates. Check your CDN/static files/customized templates, etc. Make sure the JS assets and templates are correct.

What if I see "Your ROOT_URL in app.ini is https://..... but you are visiting https://....."

Check the ROOT_URL config in your app.ini

[server]
ROOT_URL = https://.....

And check Gitea's startup logs:

AppURL(ROOT_URL): https://......

Make sure the ROOT_URL matches the base URL you are visiting.

What if I see "JavaScript error"

It could be caused by:

  • (either) incorrect template/JS assets on your side (including CDN/cache problems)
  • (or) a frontend bug in Gitea or in your customized JS code

@wxiaoguang wxiaoguang added this to the 1.17.0 milestone Mar 2, 2022
@wxiaoguang wxiaoguang added the topic/ui Change the appearance of the Gitea UI label Mar 2, 2022
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 2, 2022
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Mar 2, 2022
@wxiaoguang wxiaoguang changed the title Show messages for users if the ROOT_URL is wrong Show messages for users if the ROOT_URL is wrong, show JavaScript errors Mar 29, 2022
@wxiaoguang
Copy link
Contributor Author

@go-gitea/maintainers I think we need this PR to save all active maintainers' time.

@lunny
Copy link
Member

lunny commented Mar 29, 2022

How about to force redirect to ROOT_URL when visit from a non-ROOT_URL address?

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 29, 2022
@wxiaoguang
Copy link
Contributor Author

How about to force redirect to ROOT_URL when visit from a non-ROOT_URL address?

It will cause problems if the ROOT_URL is localhost but the user is visiting Gitea from another host.

@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Mar 29, 2022

And I think we can wait for a few days before merge after lgtm/done.

Maybe some maintainers have new opinions. 🙏

@@ -0,0 +1,44 @@
<script>
<!-- /* eslint-disable */ -->
window.addEventListener('error', function(e) {window._globalHandlerErrors=window._globalHandlerErrors||[]; window._globalHandlerErrors.push(e);});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably create a separate webpack chunk that loads beforeindex.js for this instead of inlining it. That way, you also shouldn't need this global variable.

Copy link
Contributor Author

@wxiaoguang wxiaoguang Mar 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, index.js is very late, the layout is:

window.addEventListener('error')
window.config =  // error occurs 
template <script> // error occurs
index.js 
- bootstrap.js (the old publicpath.js)
- others (initXxx)  // error occurs

So the window.addEventListener must be the first one in script tag.

@wxiaoguang wxiaoguang added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Mar 29, 2022
@wxiaoguang wxiaoguang added the skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. label Mar 29, 2022
@Gusted
Copy link
Contributor

Gusted commented Mar 29, 2022

How about to force redirect to ROOT_URL when visit from a non-ROOT_URL address?

That will be putting up a bandaid around possible incorrect configuration. To show the error seems the best option IMO. If they suddenly will be redirected to a new URL, how will they know what's going on and how to fix it?

@codecov-commenter

This comment was marked as off-topic.

@wxiaoguang wxiaoguang requested a review from silverwind March 29, 2022 13:25
@wxiaoguang
Copy link
Contributor Author

Let's merge and see user's reactions. If it helps, that's good. If some users feel annoying, we should find the real problem behind these users.

@wxiaoguang wxiaoguang merged commit 2bce1ea into go-gitea:main Mar 30, 2022
@wxiaoguang wxiaoguang deleted the detect-wrong-url branch March 30, 2022 05:52
zjjhot added a commit to zjjhot/gitea that referenced this pull request Mar 30, 2022
* giteaoffical/main: (31 commits)
  Add Package Registry (go-gitea#16510)
  Show messages for users if the ROOT_URL is wrong, show JavaScript errors (go-gitea#18971)
  [skip ci] Updated translations via Crowdin
  Make git.OpenRepository accept Context (go-gitea#19260)
  Use full output of git show-ref --tags to get tags for PushUpdateAddTag (go-gitea#19235)
  When conflicts have been previously detected ensure that they can be resolved (go-gitea#19247)
  More commit info from API (go-gitea#19252)
  Move some issue methods as functions (go-gitea#19255)
  Move project files into models/project sub package (go-gitea#17704)
  Granular webhook events in editHook (go-gitea#19251)
  Provide configuration to allow camo-media proxying (go-gitea#12802)
  Move init repository related functions to modules (go-gitea#19159)
  Move organization related structs into sub package (go-gitea#18518)
  Refactor repo clone button and repo clone links, fix JS error on empty repo page (go-gitea#19208)
  Show last cron messages on monitor page (go-gitea#19223)
  Allow API to create file on empty repo (go-gitea#19224)
  Use goproxy.io instead of goproxy.cn (go-gitea#19242)
  New cron task: delete old system notices (go-gitea#19219)
  Let web and API routes have different auth methods group (go-gitea#19168)
  Only send webhook events to active system webhooks and only deliver to active hooks (go-gitea#19234)
  ...
@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Apr 8, 2022

FYI:

Using HTMLURL in templates is the root case that Gitea can only serve web under ROOT_URL. That's why we have to force users to set ROOT_URL correctly, and that's why this PR came.

If all URLs in template can be refactored to use Relative URL correctly, then we do not need to force users set ROOT_URL correctly again and again.

@wxiaoguang
Copy link
Contributor Author

Some updates about this PR (which may be useful for future readers).

The show JavaScript errors really helps to catch some frontend bugs, including:

And

The ROOT_URL detection also helps issues like #19972 and some users in discord.


I believe it's on the right way. If there are new issues about it in the future, these related bugs could also be fixed.

@6543 6543 removed the skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. label Jun 20, 2022
@Gusted Gusted added the pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! label Jul 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! topic/ui Change the appearance of the Gitea UI type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants