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

Jumping to search results is broken when URL includes life.html instead of just life #843

Closed
davidebbo opened this issue Apr 30, 2024 · 4 comments · Fixed by #844
Closed

Comments

@davidebbo
Copy link
Contributor

davidebbo commented Apr 30, 2024

To repro:

Nothing happens, due to the following exception:

OZentry.6162883321d1a8e6a2f0.js:1 Uncaught TypeError: Failed to construct 'URL': Invalid URL
    at Bo (OZentry.6162883321d1a8e6a2f0.js:1:247653)
    at t.set_treestate (OZentry.6162883321d1a8e6a2f0.js:1:294506)
    at closeAndLeap (VM30 life.html:24:28)
    at HTMLDivElement.<anonymous> (VM30 life.html:498:19)
    at HTMLDivElement.dispatch (jquery.min.js:3:9922)
    at q.handle (jquery.min.js:3:7949)

This is caused by a bug in the "Strip /life/ from any URL" logic (

onezoom.controller.set_treestate(href.replace(/^\/[A-Z_\-/]+/i, ''));
), which doesn't expect the .html extension, and leaves the URL starting with a bogus ".html/@etc...".

@davidebbo
Copy link
Contributor Author

The fix is to simply include periods in the regex character class (square brackets).

@lentinj
Copy link
Collaborator

lentinj commented Apr 30, 2024

Maybe we should bin the regex replacement, and feed a "ignore URL base" option to set_treestate(), so we don't try and go from /life_expert -> /life (for example).

That said, I don't think set_treestate() would actually change pages if passed a full URL anyway, and maybe it would never make sense to do so?

@davidebbo
Copy link
Contributor Author

so we don't try and go from /life_expert -> /life (for example)

That would not happen, since we have underscore in the character class, no?

I was just trying to target the specific 'period' issue tactically with a minimal change, but if you think that a different change is best, we can do that instead.

@lentinj
Copy link
Collaborator

lentinj commented Apr 30, 2024

That would not happen, since we have underscore in the character class, no?

Yup, I'm not arguing against your fix, which is a good catch and worth applying.

My point is more that the whole regex-replacement will probably bite us again in the future, and could probably be binned & replaced with a special case in set_treestate() more robustly. But it's certainly not a high-priority thing.

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 a pull request may close this issue.

2 participants