refactor(link-preview): move link previews into core so standalone can serve them - #10913
Merged
Conversation
Both are pure and platform-free — ico.ts slices an icon directory without decoding anything, page_description.ts only walks a node-html-parser tree — so neither had any reason to sit in the server app beyond the route that happened to be its only caller. Moving them is the first step of making link previews runnable wherever Trilium runs rather than only where Node does. Their specs move unchanged and now execute under both test projects: apps/server (node) and apps/standalone (happy-dom), which is what proves the portability claim rather than merely asserting it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…only inspectors The route had its own copy of "keep these bytes as an attachment and answer with the URL" — the same step image_download.ts already performs for pictures a note names by address, down to the same commons helpers and the same api/attachments URL. Its doc comment even anticipated this caller. So storePictureBytes is exported instead of reimplemented, returning the attachment id alongside the URL for the one thing a preview needs and an import does not: waiting for the write to land, because the client renders the moment the route answers. Identifying a picture now goes through core's inspectImage rather than image_codec (image-type) and is-svg. That is what makes the file portable — image_codec carries Jimp and UPNG and cannot leave the server — and it is better on its own terms: the SVG sniff reads the opening 1000 bytes instead of stringifying the whole file, so the "only sniff what we would keep" caveat is gone, and the media type is no longer carried alongside the bytes when the store re-reads it anyway. Bytes travel as Uint8Array throughout, Buffer surviving only where Jimp insists on one. Two things fell out of the move rather than being sought: - page_description now types its input structurally instead of against node-html-parser's HTMLElement. apps/server never declared that dependency and had been resolving a hoisted 6.1.13 against core's 9.0.1, so the two HTMLElements were nominally different types. Core has no business dictating which copy a host app parses with. - The spec spied the barrel's imageService, which cannot intercept a core module reaching its own image service by relative import; it spies the singleton now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…times on RequestProvider could fetch JSON over Trilium's own protocol and could fetch a picture, and a link preview needs neither: it reads an arbitrary third party's page and has to bound what it will hold while doing so. fetchResource is that — bytes, the media type they came under, the status, and a ceiling that is required rather than defaulted. The counting is shared (readCappedResponse) because only the getting of the Response differs between runtimes, and the counting is the part with the edge cases: a size advertised and a size not, a body that lies about the first, a stream that must be abandoned partway rather than read to the end to discover it was too long. The address checks are shared for the same reason — they are about the URL, not the network, so validateFetchableUrl now holds what safe_fetch's validateUrl held and safe_fetch keeps only the half that needs a resolver. Three implementations, and they are honestly unequal: - Server: safeFetch, so the name is resolved, private ranges refused, and the connection pinned to the addresses actually checked. - Standalone over the native transport: the address checks, and no resolution — there is no resolver in a worker. Documented rather than papered over, along with why the server's rule does not transfer: it is about a host reached by people who are not its owner, which a phone is not. - Standalone in the page: plain fetch, so the same-origin policy decides whether the answer can be read at all. Most sites say no. That is what running with no server costs, and the caller is expected to degrade rather than retry. The six specs that hand-rolled a RequestProvider now share a fake whose unstubbed methods fail loudly, so the next method added to the interface does not break six unrelated tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ider The last thing tying the route to the server was Jimp: decode, scale to 256, ask the pixels whether any of them are transparent, re-encode. That is platform knowledge, so it moves behind ImageProvider.resizeForPreview and into image_codec, which already owns Jimp and already knows nothing about Trilium. Declining is part of the contract rather than a failure. Standalone has no decoder and says so; the caller then keeps a small cover image exactly as it arrived and shows a preview without a picture where it is too large — which is precisely what the server already did for a WebP or AVIF its own decoder cannot read. The degraded path was already written and already tested; this just gives a second runtime the same one. Kept apart from compressImage deliberately. Compression is offered a picture the user chose to keep and tries to make it cheaper; this is handed someone else's og:image, up to 5MB of it, and wants a thumbnail. Sharing the path would mean carrying the whole thing through a pipeline sized for the user's own photographs to produce something discarded at this size anyway. One behaviour improves on the way past. The minimum-dimension rule — which stops a 16x16 favicon being blown up into a card — was enforced by decoding and measuring, so a picture that would not decode was always refused as unverifiable. It reads the header instead now, via inspectImage, so a large WebP apple-touch-icon is accepted on the strength of what its header states, costs nothing to check, and is checkable on a runtime with no decoder at all. A header that states nothing is still treated as too small. Three specs stopped listing an ImageProvider's methods to wrap one of them and spread the real provider instead, which is what they meant. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The last server-only pieces were the two fetches. Both go through the request provider now, so the route registers in core's shared routes and the standalone BrowserRouter picks it up along with everything else — where before it answered 404, the client caught it, and every pasted link silently degraded to a hostname. The client needs no change at all: it has always posted to link-embed/metadata. Falling out of the move: - readResponseText is gone. It capped by characters after decoding while claiming bytes, and sliced the result to the same number a second time; the provider counts bytes as they stream, which is what it meant. - The oEmbed answer is parsed rather than handed to Response.json(), and gets a ceiling of its own — it is a handful of fields, so anything approaching 64KB is not one. The spec moves with it and now runs under both test projects. That is the point of the exercise, and it caught the one real difference immediately: standalone has no decoder, so five assertions about resizing failed there while the other thirty-five passed. Rather than let the outcome depend on which project ran it, the spec installs the resizer it is testing against, and the runtime that cannot resize gets tests of its own — a small cover kept byte-for-byte as it arrived, a large one dropped with the rest of the preview intact. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… path The two older methods on this provider await `Promise<any>`; the new one does not need to. Types the reply it actually reads, which is what the lint rule was asking for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dosubot
Bot
added
the
size:XL
Contributor
Greptile SummaryThe PR moves link-preview metadata handling into shared core so server and standalone runtimes can use the same route and processing pipeline.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| packages/trilium-core/src/routes/api/link_embed.ts | Moves link-preview fetching, parsing, image selection, and attachment storage into the portable core route. |
| packages/trilium-core/src/services/request.ts | Introduces the shared capped-resource response contract, URL validation, and streaming body limit enforcement. |
| apps/server/src/services/request.ts | Implements capped resource fetching through the server's SSRF-hardened transport. |
| apps/standalone/src/lightweight/request_provider.ts | Implements browser-based resource fetching with URL validation, timeout handling, and capped response reading. |
| apps/standalone/src/lightweight/bridged_request_provider.ts | Implements native-bridge resource fetching and rejects oversized encoded responses before decoding. |
| apps/server/src/services/image_codec.ts | Adds server-side preview resizing and format selection behind the shared image-provider abstraction. |
| packages/trilium-core/src/routes/index.ts | Registers the link-preview endpoint through the shared API route builder. |
Sequence Diagram
sequenceDiagram
participant C as Client
participant R as Shared core route
participant P as Platform RequestProvider
participant O as Remote origin
participant I as ImageProvider
participant N as Note attachments
C->>R: POST link-embed/metadata
R->>P: fetchResource(url, maxBytes)
P->>O: Fetch page and image resources
O-->>P: Capped bytes and media type
P-->>R: FetchedResource
R->>I: Inspect and resize preview images
I-->>R: Processed image bytes or fallback
R->>N: Store favicon and cover image
R-->>C: LinkEmbedMetadata
Reviews (3): Last reviewed commit: "Merge branch 'main' into refactor/link-p..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
|
🖥️ App preview is ready! 🔗 Preview URL: https://pr-10913.trilium-app.pages.dev ✅ All checks passed This preview will be updated automatically with new commits. |
Bundle ReportChanges will increase total bundle size by 8.66kB (0.01%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: client-esmAssets Changed:
view changes for bundle: standalone-esmAssets Changed:
|
Greptile is right that the ceiling was enforced too late, and the reasoning holds further than the comment above it admitted: atob yields a binary string and Uint8Array.from copies that again, so an oversized body checked afterwards has already been held three times over. The size is now read off the base64 length — exact for what the bridge produces, and an over-estimate for anything malformed, which is the safe direction for a ceiling — so an oversized response is refused without being decoded at all. What this does not do is bound what the native side and the plugin bridge already spent to deliver the body, which is where the memory in the report mostly goes. Doing that means handing the ceiling to the transport, and CapacitorHttp cannot honour one: it answers only once the whole response is in hand. The Android streaming proxy could, since it is a real fetch with a real stream, but the two are not connected today and connecting them is a change of its own. The comment now says so rather than implying the ceiling is complete. Also uses core's decodeUtf8 rather than a bare TextDecoder in the moved route, per the binary-utilities rule in CLAUDE.md. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
📚 Documentation preview is ready! 🔗 Preview URL: https://pr-10913.trilium-docs.pages.dev ✅ All checks passed This preview will be updated automatically with new commits. |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
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
1 participant
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.
Link previews were server-only. In standalone the client's
POST link-embed/metadatahit the browser router'sNot found,fetchMetadatacaught it, and every pasted link silently degraded to a hostname — with the explicit "convert to card" path inserting a hostname-only card, since it has no unresolved-page veto.This moves the feature into
trilium-coreso standalone serves it too, and cleans up what the move exposed. No client changes — it has always posted tolink-embed/metadata.What moved, and why it was cheaper than it looks
Most of the machinery was already in core.
image_download.tswas already fetching remote bytes throughRequestProvider, identifying them withinspectImage, and storing them as attachments — itsdownloadPictureToAttachmentdoc comment even names this caller ("a repair that fills in what a preview is missing"). So the route's picture-storing was deleted rather than ported.18ca5e9ico.ts+page_description.ts→ core. Both pure —icoslices an icon directory without decoding,page_descriptionwalks a parsed tree02c8320storePictureBytes; identify viainspectImage. Dropsimage-type,is-svgand theimage_codecdependency; bytes travel asUint8Array5642274RequestProvider.fetchResource— bytes, media type, status, and a required ceilingd85d4b7ImageProvider.resizeForPreview43faaf5The one genuinely new abstraction
fetchResourceexists because the two existing methods don't fit:execis JSON over Trilium's own protocol,getImagehas no ceiling and reports no media type. Reading a third party's page needs both.The counting is shared (
readCappedResponse) because only the getting of theResponsediffers per runtime — the counting is where the edge cases live. The address checks are shared (validateFetchableUrl) because they're about the URL, not the network.The three implementations are honestly unequal, and the doc comments say so:
safeFetch: name resolved, private ranges refused, connection pinned to the checked addresses.fetch, so the same-origin policy decides. Many sites say no; the caller degrades rather than retries.Verified against the live web
Booted
pnpm standalone:start, drove a real browser through setup, requested four previews:.ico— icon trimming ran in the browser)embedType: youtube, 21 KB thumbnail storedunresolved: true— sends noAccess-Control-Allow-Origin, degrades cleanlyAll four stored attachments fetched straight back at HTTP 200 with correct MIME types — the only place
awaitImageWrite's contract can actually be proven.Testing
Typecheck clean. 288 specs pass under
apps/server, 269 underapps/standalone. The route's own 42 run identically under both, which is what makes the portability claim checkable rather than asserted.That cross-project run earned its keep immediately: five assertions about resizing failed under standalone, correctly, because there is no decoder there. Rather than let the outcome depend on which project ran it, the spec installs the resizer it tests against, and the runtime that cannot resize gets tests of its own — a small cover kept byte-for-byte, a large one dropped with the rest of the preview intact. The real Jimp implementation is covered separately in
image_codec_preview.spec.ts.Behaviour changes
unresolved, as today.resizeForPreviewis the hook if real thumbnails there are wanted later (createImageBitmap+OffscreenCanvasare available in the worker).inspectImagenow — a large WebPapple-touch-iconis accepted on what its header states, costs nothing, and works where there is no decoder. A header that states nothing is still treated as too small.Worth a reviewer's judgement
A latent bug surfaced.
apps/serverusesnode-html-parserin six modules but never declares it — it had been resolving a hoisted 6.1.13 against core's 9.0.1, so the twoHTMLElements were nominally different types. Fixed here by typingfindPageDescriptionstructurally, not by aligning versions: a v6→v9 bump across the clipper, OneNote importer and share renderer is a separate change with real blast radius. The undeclared dependency is still there.Repo-wide
pnpm dev:linter-checkOOMs at 4 GB, onmainas well as on this branch. Changed files were linted individually;packages/trilium-coreturns out to be excluded from ESLint entirely.Shared test fakes. Six specs hand-rolled a
RequestProvider, so adding a method broke all six. They now sharefakeRequestProvider, whose unstubbed methods fail loudly. ThreeImageProviderfakes that listed every method to wrap one now spread the real provider, which is what they meant.🤖 Generated with Claude Code