mirror of
https://github.com/vitorpamplona/amethyst.git
synced 2026-10-06 03:38:23 +00:00
fix: harden the title/favicon link preview fallback
Audit fixes for the compact link preview:
- Stop reading <title> and icon <link>s once <body> starts. </head> is
optional, and without it an inline SVG's <title> ("Close", "Menu") was
taken as the page title.
- Never cut a long <title> inside a surrogate pair.
- Match the HTML mime type case-insensitively: UrlPreview stores the
server's spelling, so "Text/HTML" pages lost their icon.
- Honour data: icons. An inline data:image icon (up to 64 KB) is used as
is; `href="data:,"` means "no favicon", so /favicon.ico is no longer
requested for those sites.
- Remember icon URLs the server refused (HTTP errors only, not network
errors) so a missing /favicon.ico is not re-requested every time its
card scrolls back into view.
- Keep the open-in-browser callback stable across recompositions.
- Benchmark rows for the title/icon parse path: no measurable cost on
the heavy head, ~1.5 us on a small one.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KdPp7UiETpzv8jQ5qfAFqp
This commit is contained in:
+23
-3
@@ -58,6 +58,7 @@ object MetaTagsParser {
|
||||
private const val META = "meta"
|
||||
private const val LINK = "link"
|
||||
private const val HEAD = "head"
|
||||
private const val BODY = "body"
|
||||
private const val ICON = "icon"
|
||||
private const val REL = "rel"
|
||||
private const val CONTENT = "content"
|
||||
@@ -88,6 +89,12 @@ object MetaTagsParser {
|
||||
/** The `</head>` that ends the interesting part of the document. */
|
||||
HEAD_END,
|
||||
|
||||
/**
|
||||
* A `<body>` start tag. `</head>` is optional, so on a page that omits it this is the only
|
||||
* sign the head is over.
|
||||
*/
|
||||
BODY_START,
|
||||
|
||||
/** A closed `<title>`: its text is [TagScanner.textStart]..<[TagScanner.textEnd]. */
|
||||
TITLE,
|
||||
|
||||
@@ -111,26 +118,34 @@ object MetaTagsParser {
|
||||
): Sequence<MetaTag> =
|
||||
sequence {
|
||||
val s = TagScanner(input)
|
||||
// Meta tags past an implicit end of head are still read, as they always were. The
|
||||
// title and icons are not: past `<body>`, a `<title>` is an inline SVG's tooltip
|
||||
// ("Close", "Menu"), not the page's name.
|
||||
var inBody = false
|
||||
while (!s.exhausted()) {
|
||||
when (s.nextTag()) {
|
||||
TagKind.HEAD_END -> {
|
||||
break
|
||||
}
|
||||
|
||||
TagKind.BODY_START -> {
|
||||
inBody = true
|
||||
}
|
||||
|
||||
TagKind.META -> {
|
||||
val attrs = parseAttrs(input, s.attrsStart, s.attrsEnd) ?: continue
|
||||
yield(MetaTag(attrs))
|
||||
}
|
||||
|
||||
TagKind.TITLE -> {
|
||||
if (includeTitleAndIcons) {
|
||||
if (includeTitleAndIcons && !inBody) {
|
||||
val text = titleText(input, s.textStart, s.textEnd)
|
||||
if (text.isNotEmpty()) yield(MetaTag(mapOf(CONTENT to text), HeadElement.TITLE))
|
||||
}
|
||||
}
|
||||
|
||||
TagKind.LINK -> {
|
||||
if (includeTitleAndIcons) {
|
||||
if (includeTitleAndIcons && !inBody) {
|
||||
val attrs = parseAttrs(input, s.attrsStart, s.attrsEnd) ?: continue
|
||||
if (isIconRel(attrs[REL])) yield(MetaTag(attrs, HeadElement.LINK))
|
||||
}
|
||||
@@ -160,7 +175,11 @@ object MetaTagsParser {
|
||||
): String {
|
||||
val raw = input.substring(from, to)
|
||||
val decoded = if (raw.indexOf('&') < 0) raw else raw.replace(Attrs.RE_CHAR_REF, Attrs.Companion::replaceCharRefs)
|
||||
return decoded.replace(WHITESPACE_RUN, " ").trim().take(MAX_TITLE_LENGTH)
|
||||
val title = decoded.replace(WHITESPACE_RUN, " ").trim()
|
||||
if (title.length <= MAX_TITLE_LENGTH) return title
|
||||
// Never end on half of a surrogate pair: a lone high surrogate renders as tofu.
|
||||
val cut = if (title[MAX_TITLE_LENGTH - 1].isHighSurrogate()) MAX_TITLE_LENGTH - 1 else MAX_TITLE_LENGTH
|
||||
return title.substring(0, cut)
|
||||
}
|
||||
|
||||
private class TagScanner(
|
||||
@@ -314,6 +333,7 @@ object MetaTagsParser {
|
||||
when (nameEnd - nameStart) {
|
||||
META.length -> {
|
||||
if (nameIs(nameStart, nameEnd, META)) return TagKind.META
|
||||
if (nameIs(nameStart, nameEnd, BODY)) return TagKind.BODY_START
|
||||
// Most `<link>`s are stylesheets and preloads; checking the raw span for `icon`
|
||||
// keeps their attributes from ever being parsed into a map.
|
||||
if (nameIs(nameStart, nameEnd, LINK) && spanContains(attrsStart, attrsEnd, ICON)) return TagKind.LINK
|
||||
|
||||
+15
-1
@@ -71,10 +71,20 @@ class UrlInfoItem(
|
||||
/**
|
||||
* The page's icon, absolute: the one it declared, else `/favicon.ico` at its origin -- the
|
||||
* same guess every browser makes, since most sites that declare nothing still serve one.
|
||||
*
|
||||
* A `data:` icon is used as-is when it is an image (the image loader decodes those inline),
|
||||
* and taken at its word when it is not: `href="data:,"` is how a site tells browsers it has no
|
||||
* favicon, so guessing `/favicon.ico` there is a request that is known to miss.
|
||||
*
|
||||
* Null for a non-HTML URL (an image or a video is its own preview) or an unresolvable one.
|
||||
*/
|
||||
val iconUrlFullPath: String? =
|
||||
if (mimeType.startsWith("text/html")) resolve(icon) ?: resolve(DEFAULT_FAVICON) else null
|
||||
when {
|
||||
!mimeType.startsWith("text/html", ignoreCase = true) -> null
|
||||
icon.startsWith("data:image/", ignoreCase = true) -> if (icon.length <= MAX_INLINE_ICON_LENGTH) icon else resolve(DEFAULT_FAVICON)
|
||||
icon.startsWith("data:", ignoreCase = true) -> null
|
||||
else -> resolve(icon) ?: resolve(DEFAULT_FAVICON)
|
||||
}
|
||||
|
||||
/**
|
||||
* The declared `og:video`, absolute, but only when the declaration holds up: a [videoType] the
|
||||
@@ -149,5 +159,9 @@ class UrlInfoItem(
|
||||
|
||||
companion object {
|
||||
private const val DEFAULT_FAVICON = "/favicon.ico"
|
||||
|
||||
// Previews are cached, so an inline icon is held for as long as its card is. Real inline
|
||||
// favicons are a few KB; anything far past that is not worth keeping in memory.
|
||||
private const val MAX_INLINE_ICON_LENGTH = 64 * 1024
|
||||
}
|
||||
}
|
||||
|
||||
+34
@@ -176,4 +176,38 @@ class TitleAndIconFallbackTest {
|
||||
|
||||
assertEquals("", info.icon)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun aBodyTitleIsIgnoredWhenTheHeadIsNeverClosed() {
|
||||
// `</head>` is optional: `<body>` closes the head implicitly. An inline SVG's <title>
|
||||
// ("Close", "Menu") is the classic thing to then mistake for the page title.
|
||||
val info = extract("""<head><meta name="description" content="D"><body><svg><title>Close</title></svg></body>""")
|
||||
|
||||
assertEquals("", info.title)
|
||||
assertEquals("D", info.description)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun aBodyIconLinkIsIgnoredWhenTheHeadIsNeverClosed() {
|
||||
val info = extract("""<head><title>T</title><body><link rel="icon" href="/late.png"></body>""")
|
||||
|
||||
assertEquals("T", info.title)
|
||||
assertEquals("", info.icon)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun aTitleWithAttributesIsRead() {
|
||||
val info = extract("""<head><title data-rh="true">Helmet</title></head>""")
|
||||
|
||||
assertEquals("Helmet", info.title)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun aLongTitleIsNotCutInsideASurrogatePair() {
|
||||
// 299 ASCII chars then an emoji: a plain take(300) keeps only its high surrogate.
|
||||
val info = extract("<head><title>" + "a".repeat(299) + "\uD83D\uDE00 tail</title></head>")
|
||||
|
||||
assertTrue(info.title.isNotEmpty())
|
||||
assertTrue(!info.title.last().isHighSurrogate(), "title ends in a dangling high surrogate")
|
||||
}
|
||||
}
|
||||
|
||||
+22
@@ -84,4 +84,26 @@ class UrlInfoItemTextPreviewTest {
|
||||
fun nonHtmlUrlsHaveNoIcon() {
|
||||
assertNull(page(url = "https://example.com/a.png", image = "https://example.com/a.png", mimeType = "image/png").iconUrlFullPath)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun anUppercaseHtmlMimeStillGetsAnIcon() {
|
||||
// UrlPreview stores MediaType.toString(), which keeps the server's spelling.
|
||||
assertEquals(
|
||||
"https://example.nsite.lol/favicon.ico",
|
||||
page(mimeType = "Text/HTML; charset=UTF-8").iconUrlFullPath,
|
||||
)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun anEmptyDataIconMeansTheSiteHasNoIcon() {
|
||||
// `<link rel="icon" href="data:,">` is the common way to tell browsers not to request
|
||||
// /favicon.ico. Requesting it anyway is a guaranteed miss.
|
||||
assertNull(page(icon = "data:,").iconUrlFullPath)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun anInlineImageIconIsUsedAsIs() {
|
||||
val inline = "data:image/png;base64,iVBORw0KGgo="
|
||||
assertEquals(inline, page(icon = inline).iconUrlFullPath)
|
||||
}
|
||||
}
|
||||
|
||||
+13
@@ -160,6 +160,19 @@ class MetaTagsParserBenchmark {
|
||||
}
|
||||
bench("no </head>, whole doc", open, 2_000) { MetaTagsParser.parse(it).count() }
|
||||
|
||||
// What production runs (HtmlParser asks for the <title> and icon <link>s too). Should sit
|
||||
// on top of the rows above: the 60 preload links never reach the attribute parser.
|
||||
bench("spa head, +title/icons", spa, 50_000) { MetaTagsParser.parse(it, includeTitleAndIcons = true).count() }
|
||||
bench("spa head, og: + fallbacks", spa, 50_000) {
|
||||
OpenGraphParser().extractUrlInfo(MetaTagsParser.parse(it, includeTitleAndIcons = true)).title.length
|
||||
}
|
||||
bench("heavy head, +title/icons", heavy, 2_000) { MetaTagsParser.parse(it, includeTitleAndIcons = true).count() }
|
||||
bench("no </head>, +title/icons", open, 2_000) { MetaTagsParser.parse(it, includeTitleAndIcons = true).count() }
|
||||
|
||||
val spaFallbacks = OpenGraphParser().extractUrlInfo(MetaTagsParser.parse(spa, includeTitleAndIcons = true))
|
||||
assertEquals("Example — Your Network. Your Rules.", spaFallbacks.title)
|
||||
assertEquals("/favicon.svg", spaFallbacks.icon)
|
||||
|
||||
assertTrue(MetaTagsParser.parse(open).count() >= 64)
|
||||
}
|
||||
}
|
||||
|
||||
+22
-2
@@ -20,6 +20,7 @@
|
||||
*/
|
||||
package com.vitorpamplona.amethyst.commons.ui.components
|
||||
|
||||
import androidx.collection.LruCache
|
||||
import androidx.compose.foundation.ExperimentalFoundationApi
|
||||
import androidx.compose.foundation.background
|
||||
import androidx.compose.foundation.combinedClickable
|
||||
@@ -49,6 +50,7 @@ import androidx.compose.ui.platform.LocalUriHandler
|
||||
import androidx.compose.ui.text.style.TextOverflow
|
||||
import androidx.compose.ui.unit.dp
|
||||
import coil3.compose.AsyncImage
|
||||
import coil3.network.HttpException
|
||||
import com.vitorpamplona.amethyst.commons.icons.symbols.Icon
|
||||
import com.vitorpamplona.amethyst.commons.icons.symbols.MaterialSymbols
|
||||
import com.vitorpamplona.amethyst.commons.preview.UrlInfoItem
|
||||
@@ -130,7 +132,15 @@ fun UrlPreviewCard(
|
||||
|
||||
// Only meaningful when the card's own tap does something else (e.g. opening the comment
|
||||
// thread); otherwise it would duplicate the card's open-in-browser tap.
|
||||
val onOpenInBrowser: (() -> Unit)? = onCardClick?.let { { runCatching { uri.openUri(url) } } }
|
||||
val hasCardClick = onCardClick != null
|
||||
val onOpenInBrowser: (() -> Unit)? =
|
||||
remember(hasCardClick, url, uri) {
|
||||
if (hasCardClick) {
|
||||
{ runCatching { uri.openUri(url) } }
|
||||
} else {
|
||||
null
|
||||
}
|
||||
}
|
||||
|
||||
// A page with no picture to lead with -- most often one with no OpenGraph at all, previewed
|
||||
// from its `<title>`, meta description and favicon -- gets the short, wide card. Painting the
|
||||
@@ -251,6 +261,13 @@ private fun CompactUrlPreviewCard(
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Icon URLs the server refused (404 and the like) in this process. The image loader caches
|
||||
* successes but not failures, so without this a site with no `/favicon.ico` would be asked for it
|
||||
* again -- and 404 again -- every time its card scrolled back into view.
|
||||
*/
|
||||
private val failedIconUrls = LruCache<String, Unit>(200)
|
||||
|
||||
/**
|
||||
* The site's icon in a fixed square. A generic globe sits there until the icon actually loads,
|
||||
* so a site whose favicon is missing (the `/favicon.ico` guess is only a guess) or undecodable
|
||||
@@ -278,13 +295,16 @@ private fun UrlPreviewIcon(
|
||||
)
|
||||
}
|
||||
|
||||
if (iconUrl != null) {
|
||||
if (iconUrl != null && failedIconUrls[iconUrl] == null) {
|
||||
AsyncImage(
|
||||
model = iconUrl,
|
||||
contentDescription = contentDescription,
|
||||
contentScale = ContentScale.Fit,
|
||||
modifier = UrlPreviewIconImageModifier,
|
||||
onSuccess = { loaded = true },
|
||||
// Only a server's answer is remembered. A network error (offline, a timeout) says
|
||||
// nothing about the icon and must be retried once the connection is back.
|
||||
onError = { if (it.result.throwable is HttpException) failedIconUrls.put(iconUrl, Unit) },
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user