fix: use referrer for toolbar return link when available#1501
fix: use referrer for toolbar return link when available#1501ParthAggarwal16 wants to merge 1 commit intointernetarchive:masterfrom
Conversation
|
Hi! Is this still being worked on @ParthAggarwal16 ? |
|
Hello @MarcCoquand , this is ready to review from my side. please let me know if any changes are needed |
|
It looks good to me |
There was a problem hiding this comment.
Pull request overview
Updates BookReader’s toolbar “return” link so it can point back to the user’s actual previous page (via document.referrer) instead of always using a static bookUrl, addressing the navigation break described in #1494.
Changes:
- Add logic to prefer
document.referrerwhen present and same-origin. - Fall back to
br.bookUrlif referrer is absent, cross-origin, or unparsable.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (br.bookUrl && br.options.enableBookTitleLink) { | ||
| // Use referrer if available and from same origin, otherwise use bookUrl | ||
| // This allows returning to the previous BookServer Explorer App page | ||
| let returnUrl = br.bookUrl; | ||
| const referrer = document.referrer; | ||
|
|
||
| if (referrer) { | ||
| try { | ||
| const referrerUrl = new URL(referrer); | ||
| const currentUrl = new URL(window.location.href); | ||
| if (referrerUrl.origin === currentUrl.origin) { | ||
| returnUrl = referrer; | ||
| } |
There was a problem hiding this comment.
The new return-link behavior (preferring document.referrer when same-origin) isn’t covered by tests. Since this repo already has Jest tests for Toolbar.js, add unit tests for buildToolbarElement() covering: (1) defaulting to br.bookUrl, (2) same-origin referrer override, (3) cross-origin referrer ignored, and (4) invalid referrer string fallback.
| if (referrerUrl.origin === currentUrl.origin) { | ||
| returnUrl = referrer; | ||
| } | ||
| } catch (e) { |
There was a problem hiding this comment.
catch (e) introduces an unused variable, which violates this repo’s no-unused-vars ESLint rule and will fail npm run lint. Use an empty catch binding (catch { ... }) or otherwise reference the error variable.
| } catch (e) { | |
| } catch { |
| const referrer = document.referrer; | ||
|
|
||
| if (referrer) { |
There was a problem hiding this comment.
There is trailing whitespace on the blank line after const referrer = document.referrer;, which violates the repo’s no-trailing-spaces ESLint rule and will fail npm run lint. Remove the whitespace (or the blank line).
| } | ||
|
|
||
| $titleSectionEl.append( |
There was a problem hiding this comment.
There is trailing whitespace on the blank line just before appending the <a> element, which violates the repo’s no-trailing-spaces ESLint rule and will fail npm run lint. Remove the whitespace (or the blank line).
fixes the toolbar return link to use the referrer when available instead of a static URL
This change updates the toolbar to use document.referrer when it’s available, ensuring that the return link directs the user back to the actual previous page
closes #1494