Skip to content

Make Copy Link to Highlight links more reslient to url encoding#1562

Open
cdrini wants to merge 2 commits into
internetarchive:masterfrom
cdrini:fix/clth-reliable-encoding
Open

Make Copy Link to Highlight links more reslient to url encoding#1562
cdrini wants to merge 2 commits into
internetarchive:masterfrom
cdrini:fix/clth-reliable-encoding

Conversation

@cdrini

@cdrini cdrini commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Sharing a "Copy link to highlight" link on eg facebook messenger resulted in the parameters being re-encoded:

Input link: https://archive.org/details/macklife0000mack/page/246/mode/2up?text=246:to%20some%20extent%20%E2%80%98silly%E2%80%99.-,But%20I%20can,We%E2%80%99re%20all%20%E2%80%98them%E2%80%99.,-Lee%20Mack

Link in facebooks messenger: https://archive.org/details/macklife0000mack/page/246/mode/2up?text=246%3Ato%20some%20extent%20%E2%80%98silly%E2%80%99.-%2CBut%20I%20can%2CWe%E2%80%99re%20all%20%E2%80%98them%E2%80%99.%2C-Lee%20Mack

Note the parameter becomes fully url encoded, with eg : being changed to %3A , and also , being replaced.

This broke the rather brittle custom url encoding the text parameter was using. Switched to a technique where the delimiter characters are removed from the source text, so the entire string can safely be encoded as a whole!

Test here: https://deploy-preview-1562--ia-bookreader.netlify.app/details/theworksofplato01platiala

@cdrini cdrini force-pushed the fix/clth-reliable-encoding branch from 64e871b to 0d1c686 Compare June 26, 2026 13:47
@codecov

codecov Bot commented Jun 26, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 83.09859% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.68%. Comparing base (65d2212) to head (7569a0b).

Files with missing lines Patch % Lines
src/util/TextSelectionManager.js 83.33% 10 Missing and 1 partial ⚠️
src/plugins/plugin.text_selection.js 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1562      +/-   ##
==========================================
+ Coverage   63.53%   63.68%   +0.15%     
==========================================
  Files          67       67              
  Lines        6197     6185      -12     
  Branches     1376     1374       -2     
==========================================
+ Hits         3937     3939       +2     
+ Misses       2221     2207      -14     
  Partials       39       39              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
Sharing the previous link on facebook messenger resulted in the
parameters being re-encoded, which broke the rather brittle custom url
encoding the text parameter was using. Switched to a technique where the
delimeter characters are removed from the source text, so the entire
string can safely be encoded as a whole!

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@cdrini cdrini force-pushed the fix/clth-reliable-encoding branch from 0d1c686 to 8923338 Compare June 26, 2026 13:53
@cdrini cdrini requested a review from schu96 June 26, 2026 13:53

@schu96 schu96 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@cdrini I just tested using this link and it doesn't seem to be working from just a direct copy link to highlight paste into the web console. Might need to do some more testing on what is causing it to not match

https://deploy-preview-1562--ia-bookreader.netlify.app/details/theworksofplato01platiala/page/70/mode/2up?text=70:Yes%20%22%20he-,replied.%2043.%20%22And,each%20other%20and,-be%20subject%20to

Image
@schu96

schu96 commented Jun 27, 2026

Copy link
Copy Markdown
Collaborator

Confirmed that the paragraph does highlight on the current master branch
image

@schu96 schu96 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looking good for the original reported issue

Image

FYI, I was able to find an edge case where the highlighted text is not unique but the prefix/suffix should be able to help make it less ambiguous

http://127.0.0.1:8000/BookReaderDemo/demo-internetarchive.html?ocaid=theworksofplato01platiala&text=74:we%20know%20what%20it%20is%20itself%3F%22-,%22%20Certainly%20%22%20he%20replied.#page/74/mode/2up

Image
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants