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.

Sign up for GitHub

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

Jump to bottom

Search: display of search results is broken on PR builds #10714

Closed
andreasabel opened this issue Sep 6, 2023 · 8 comments
Closed

Search: display of search results is broken on PR builds #10714

andreasabel opened this issue Sep 6, 2023 · 8 comments
Labels
Bug A bug Needed: design decision A core team decision is required

Comments

@andreasabel
Copy link

First, big thanks! for providing this doc building & hosting service which we use for the Agda programming language.

I noticed that the display of search results does not fully work on PR builds of the documentation. E.g. here I am searching for word "instance" in the build for PR agda/agda#6821:
https://agda--6821.org.readthedocs.build/en/6821/search.html?q=instance&check_keywords=yes&area=default
Using Google Chrome, this only displays the sections which contain the hits but not the hits themselves.
Turning on the JavaScript Console, I see errors thrown in seachtools.js:

searchtools.js:97 GET https://agda--6821.org.readthedocs.build/en/6821/undefinedtools/command-line-options.html
searchtools.js:97 GET https://agda--6821.org.readthedocs.build/en/6821/undefinedlanguage/generalization-of-declared-variables.html 404
searchtools.js:167 Uncaught (in promise) TypeError: Cannot read properties of null (reading 'textContent')
at Object.htmlToText (searchtools.js:167:53)
at Object.makeSearchSummary (searchtools.js:553:25)
at searchtools.js:102:20

Screenshot 2023-09-06 at 04 36 46

It appears that the URLs are broken because some undefined sneaked in. Consequently, the htmlToText gets null rather than the file content and crashes.

Following the exception into the code of searchtools.js I see code that builds the URL (requestUrl) by concatenation of some strings.

  if (docBuilder === "dirhtml") {
    // dirhtml builder
    let dirname = docName + "/";
    if (dirname.match(/\/index\/$/))
      dirname = dirname.substring(0, dirname.length - 6);
    else if (dirname === "index/") dirname = "";
    requestUrl = contentRoot + dirname;
    linkUrl = requestUrl;
  } else {
    // normal html builders
    requestUrl = contentRoot + docName + docFileSuffix;
    linkUrl = docName + docLinkSuffix;
  }

Likely, one of these strings is undefined in PR-builds when it should be empty.
E.g. the valid link is https://agda--6821.org.readthedocs.build/en/6821/tools/command-line-options.html whereas GET tries https://agda--6821.org.readthedocs.build/en/6821/undefinedtools/command-line-options.html .

In the regular build everything works as expected, so the output of this query is what I expect also on the PR build:
https://agda.readthedocs.io/en/latest/search.html?q=instance&check_keywords=yes&area=default
@humitos
Copy link
Member

humitos commented Sep 6, 2023

Currently, we are not indexing files when building from pull requests due to the processing cost. However, we've talked about enabling this feature and it may happen in the future.

I think we have an exact issue for this, but I didn't find it / cc @stsewd

@andreasabel
Copy link
Author

I looked through the open issues first but couldn't find a match, so I reported this issue here.
I think I only discovered it because this PR was exactly about fixing the Search page for our docs, so naturally I tested Search on the PR build. (Usually, I just use the PR build to check the rendering of the RST.)

@stsewd
Copy link
Member

stsewd commented Sep 6, 2023

Server side search is not enabled for PR builds, but the normal Sphinx search should work, so this is a bug.

@stsewd stsewd added the Bug A bug label Sep 6, 2023
@andreasabel andreasabel mentioned this issue Sep 27, 2023
Closed
@humitos
Copy link
Member

humitos commented Feb 21, 2024

We are moving away from the "auto injecting our search" concept (see readthedocs/addons#72) and leaning more towards using our search via the addons. I don't think we will solve this issue and it can probably be closed as not planned.

@humitos humitos added the Needed: design decision A core team decision is required label Feb 21, 2024
@medmunds
Copy link

I am seeing this same behavior ("undefined" breaking search result links) in non-PR builds. It seems to depend on the specific query. Examples:

At least in my case, the broken links are caused by (my docs/requirements.txt) trying to use Sphinx 7.2.x with sphinx-rtd-theme 1.3.x. There was a breaking change in the Sphinx 7.2 searchtools.js that requires data-content_root set on the <html> element. The necessary fix to sphinx-rtd-theme's layout.html template was added in readthedocs/sphinx_rtd_theme#1507 and released in sphinx-rtd-theme 2.0.0rc2.

@humitos
Copy link
Member

humitos commented Mar 4, 2024

At least in my case, the broken links are caused by (my docs/requirements.txt) trying to use Sphinx 7.2.x with sphinx-rtd-theme 1.3.x.

It seems you should upgrade the Read the Docs Sphinx theme to 2.x. Can you do that and let us know if this fixes the problem?

@medmunds
Copy link

medmunds commented Mar 5, 2024

Yes, upgrading to sphinx-rtd-theme 2.0 solved the broken search result links for my project:

At the time the original issue here was reported, sphinx-rtd-theme 2.0 hadn't yet been released. It's likely that release on 2023-11-27 solved the specific bug reported here, for anyone not pinning docs requirements. (So this should maybe be closed fixed rather than not planned.)

Additional info for anyone else still seeing broken Read the Docs search result links with "undefined" in the URLs, perhaps using other Sphinx themes or custom layouts:

  • The problem isn't unique to PR builds; it's related to mismatched theme templates and Sphinx versions.
  • The broken links only occur with searches where the browser console shows "Read the Docs search failed. Falling back to Sphinx search." (I guess this is always the case for PR builds, but the fallback to Sphinx search may also occur for some query strings on mainline builds, too.)
  • The specific problem is that with Sphinx 7.2+, the base template for your search results (usually layout.html, usually found in your theme) must have <html ... data-content_root="{{ content_root }}">. If the data-content_root attribute is missing from the <html> tag, Sphinx's searchtools.js will construct broken search result urls containing "undefined".
  • Another symptom is that the snippets will be missing from the search results (for queries that fall back to Sphinx search).

@humitos
Copy link
Member

humitos commented Mar 5, 2024

I'm happy that sphinx-rtd-theme 2.0 solved the issue. Also, thanks a lot for the detailed description here and how to detect the symptoms.

@humitos humitos closed this as completed Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug A bug Needed: design decision A core team decision is required
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@humitos @medmunds @andreasabel @stsewd