fix(bookmarks): keep explicit title over scraped title - #426
Merged
Conversation
An explicit title on POST /api/bookmark was thrown away whenever the url turned out to be scrapable, so a scrapable bookmark could never be given a custom title. addBookmark now resolves the title in order of precedence: the title the user gave us, then the scraped page title, then the host of the url. Clients default the title field to the url itself, so a title equal to the url is treated as unset and the scraped title wins. The scrape and the title resolution are split into scrape(), resolveTitle() and hasExplicitTitle(). The page is still fetched, indexed in Typesense and screenshotted when the title is explicit, only the title is left alone. This also removes an NPE when a null title met a failed scrape, and a debug log that dereferenced retDoc.connection().response(). Tests cover a scrapable request with no title (null, empty, blank and url), with an explicit title, with a page that has no title of its own, and a failed scrape falling back to the host. The updateBookmark controller test asserted the old behaviour and now expects the posted title to be kept.
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
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.
An explicit title on
POST /api/bookmarkwas thrown away whenever the url turned out to bescrapable, so a scrapable bookmark could never be given a custom title.
addBookmarknow resolves the title in order of precedence: the title the user gave us, then thescraped page title, then the host of the url. Clients default the title field to the url itself
(
NewBookmarkCard.tsxsendstitle || url), so a title equal to the url is treated as unset andthe scraped title wins.
The scrape and the title resolution are split into
scrape(),resolveTitle()andhasExplicitTitle(). The page is still fetched, indexed in Typesense and screenshotted when thetitle is explicit, only the title is left alone.
Issue number: resolves #404
Checklist
What is the current behavior?
BookmarkService.addBookmarkoverwrites the requested title with the scraped<title>whenever theurl is scrapable. The title from the request is only used as a fallback when the page has no title
or the url is not scraped at all, so there is no way to save a scrapable bookmark under a name of
your own choosing.
What is the new behavior?
requests still get the scraped title.
screenshotted.
Does this introduce a breaking change?
The request and response shapes are unchanged. A client that sent a title and relied on the scrape
replacing it will now keep its own title, which is the bug being fixed. Stored bookmarks are
untouched.
Other information
BookmarkServiceTestcover a scrapable request with no title (null, empty,blank and the url), with an explicit title, with a page that has no title of its own, and a failed
scrape falling back to the host.
Jsoup.connectis stubbed withmockStatic, so no network isneeded.
updateBookmarktest inBookmarkControllerTestasserted the old behaviour (the scrapedLogRocket title replacing the posted
"Dark mode guide"); it now expects the posted title to bekept.
nulltitle met a failed scrape, and adebug log that dereferenced
retDoc.connection().response(), which isnullfor any document notproduced by a live connection.
spotlessCheckis clean for the three files in this PR. It still reports pre-existing violationsin
FindFirstApplication.java,SecSecurityConfig.javaandUserController.java, which this PRdeliberately leaves alone.
gradlew test --tests "*BookmarkServiceTest"(10 tests, all passing) andgradlew assemble. The Testcontainers integration tests and docker compose were not run here, noDocker on this machine.