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

Fails loading unqualified URLS with a path component #19

Closed
nicwaller opened this issue Jun 25, 2022 · 10 comments
Closed

Fails loading unqualified URLS with a path component #19

nicwaller opened this issue Jun 25, 2022 · 10 comments
Assignees
Labels
bug Something isn't working

Comments

@nicwaller
Copy link

nicwaller commented Jun 25, 2022

When I try opening a URL that is both unqualified (no gemini:// prefix) and pathed (has /astro.gmi as a suffix) I find that astro fails to render the page. When I exit astro, I see this error in the terminal:

$ ./astro derelict.garden/astro.gmi
rm: cannot remove '/home/nic/.cache/astro/links': No such file or directory

It also affects URLs opened from within astro. Press o then derelict.garden/astro.gmi.

Debug Mode

Running astro in debug mode yields some insight.

Passing case:

Starting with gemini://derelict.garden/astro.gmi
 - margin:     8
Parsing: gemini://derelict.garden/astro.gmi
Parsed URL: proto: gemini host: derelict.garden path: /astro.gmi

Failing case:

Starting with derelict.garden/astro.gmi
 - margin:     8
Parsing: derelict.garden/astro.gmi
Parsed URL: proto:  host:  path: derelict.garden/astro.gmi
Response: proto: gemini host: derelict.garden/astro.gmi port: 1965 path:

Affected Versions

Reproducible for all versions versions from v0.8.0 onward. Most recently observed with v0.14.1 0642554.

I went back to 0.7.2 (ddb79dd) and observed that the page rendered properly when invoked as shown above. The bug with this kind of URL seems to be present in all versions after that.

However, using the same version 0.7.2, I got an error trying to open derelict.garden/astro.gmi from within astro.

Passing cases

For completeness, all of the following test cases work correctly and without error. ✅

./astro gemini://derelict.garden/astro.gmi
./astro gemini://derelict.garden
./astro derelict.garden
@blmayer blmayer added the bug Something isn't working label Jun 26, 2022
@blmayer blmayer self-assigned this Jun 26, 2022
@blmayer
Copy link
Owner

blmayer commented Jun 26, 2022

Please take a look at #21. It passed your tests.

@nicwaller
Copy link
Author

nicwaller commented Jun 26, 2022

Please take a look at #21. It passed your tests.

I tested out a772b4a and confirmed that astro now loads and renders a page with any URL I can throw at it.

🐛 However! It seems to introduce a new bug with relative link navigation. For example, if I open astro like this:

./astro derelict.garden

And then I try following the link to astro browser (g + 1 + Enter) I just end up back at the same page.

@blmayer
Copy link
Owner

blmayer commented Jun 26, 2022

I got that too, but g+1 is astro.png, which doesn't exist on server, so it returns 51, and the previous page is loaded. So I'll fix it by uploading the image.

@blmayer
Copy link
Owner

blmayer commented Jun 27, 2022

You're right, I was trying the wrong link.

@blmayer
Copy link
Owner

blmayer commented Jun 27, 2022

There's a new commit on the fix, to fix the fix :-)

@blmayer
Copy link
Owner

blmayer commented Jun 28, 2022

Thanks @nicwaller for bringing attention to this bug. As the last commit passed your tests I plan to merge them and close this issue in the following day if you're OK with the solution.

@nicwaller
Copy link
Author

Thanks @nicwaller for bringing attention to this bug. As the last commit passed your tests I plan to merge them and close this issue in the following day if you're OK with the solution.

I took a look and posted my feedback on the PR thread. It seems not quite ready yet. 😐

URL handling is really challenging, more than I expected! I think adding in some unit tests for URL handling might be warranted. Want me to send a PR with some unit tests?

@blmayer
Copy link
Owner

blmayer commented Jun 28, 2022

Thanks for the feedback on the PR.

URL handling is really challenging, more than I expected! I think adding in some unit tests for URL handling might be warranted. Want me to send a PR with some unit tests?

Yes, that will be great.

@blmayer
Copy link
Owner

blmayer commented Jun 30, 2022

Finally fixed in #21. Thank you for the help. Feel free to open other issues if you find anything.

@blmayer blmayer closed this as completed Jun 30, 2022
@nicwaller
Copy link
Author

Finally fixed in #21. Thank you for the help. Feel free to open other issues if you find anything.

🥂 Cheers! Happy to help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants