Enhanced Non UTF8 HTML Support #261
Closed
+1,023
−192
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- go.mod: Language not supported
Comments suppressed due to low confidence (2)
pkg/models/url.go:376
- The removal of the idna.ToASCII conversion for hostnames (present in the original code) may lead to issues with non-ASCII domain names. Consider reintroducing the IDNA conversion to ensure proper handling of internationalized hostnames.
switch urlCopy.Host {
internal/pkg/postprocessor/extractor/html.go:121
- Filtering out any URL containing a '%' may inadvertently exclude valid percent-encoded URLs. Consider revising this filter to avoid excluding legitimate URLs.
if strings.Contains(url, "%") ||
Comment on lines
+283
to
285
| if err == nil { | ||
| addRawAsset(escapedLink) | ||
| } |
There was a problem hiding this comment.
[nitpick] When URL unquoting fails in the script tag extraction block, the error is silently ignored. Consider logging or handling this error to aid debugging without suppressing potentially important information.
Suggested change
| if err == nil { | |
| addRawAsset(escapedLink) | |
| } | |
| if err != nil { | |
| log.Warnf("Failed to unquote script link %q: %v", scriptLink, err) | |
| continue | |
| } | |
| addRawAsset(escapedLink) |
Copilot uses AI. Check for mistakes.
CorentinB
reviewed
Apr 10, 2025
| _, err := u.body.Seek(0, io.SeekStart) | ||
| if err != nil { | ||
| panic(err) | ||
| slog.Warn("failed to rewind body", "error", err) |
There was a problem hiding this comment.
An error here shouldn't happen, right? Why removing the panic?
|
Thank you! Continue in #370. :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Copied from #253:
Follows the recommended tips and trick to provide better support for different character encodings and character sets in HTML pages.
This adds additional test cases in
html_test.go, makes changes tohtml.goto improve maintainability and robustness where applicable and adds methods tomodels/url.goto handle different character encodings. Test cases have also been changed inhtml_test.goto consider the assets that are extracted rather than purely the number of them that are extracted.I'm open to any feedback
I also tried to take into consideration the comments mentioned on that PR to not manually use
idnato convert the URLs and hostnames to ASCII. I've also tried to include the changes I noticed on commit 93f6658 and e2b245b tohtml.gowhich would have caused a merge conflict on my original PR.Attempts to close #169.