Open linked images in-app on web and desktop - #688
Conversation
There was a problem hiding this comment.
🔵 Needs a closer look
Preserve external handling for matrix: and other supported custom schemes in desktop link routing.
Pull request overview
Adds in-app linked-image viewing for web and desktop, plus scoped Tauri handling for external links.
Changes:
- Adds linked-image detection, caching, fullscreen viewing, retry, and focus restoration.
- Separates preview-thumbnail clicks from article navigation.
- Adds Tauri opener integration, permissions, and regression tests.
File summaries
| File | Description |
|---|---|
src/stores/linkedImage.ts |
Stores linked-image viewer state. |
src/lib/imageLink.ts |
Detects supported image URLs and sources. |
src/lib/imageLink.test.ts |
Tests image URL handling. |
src/features/room/urlPreviews/UrlPreviewCard.tsx |
Separates thumbnail viewing from title navigation. |
src/features/room/urlPreviews/previewCache.ts |
Reuses cached preview data. |
src/features/room/urlPreviews/LinkedImageViewer.tsx |
Provides the global linked-image viewer. |
src/features/room/urlPreviews/LinkedImageViewer.browser.test.tsx |
Tests viewer and desktop flows. |
src/features/room/timeline/ImageLightbox.tsx |
Supports external URLs, retries, and encrypted media. |
src/features/room/timeline/ImageLightbox.test.tsx |
Tests lightbox behavior. |
src/app/Layout.tsx |
Mounts the linked-image viewer. |
src/app/externalLinks.ts |
Routes desktop external links through Tauri. |
src/app/externalLinks.test.ts |
Tests external-link routing. |
src/app/App.tsx |
Installs external-link delegation. |
desktop/src-tauri/src/lib.rs |
Registers the opener plugin. |
desktop/src-tauri/Cargo.toml |
Adds the opener dependency. |
desktop/src-tauri/Cargo.lock |
Locks opener dependencies. |
desktop/src-tauri/capabilities/default.json |
Scopes permitted external URL schemes. |
Review details
Suppressed comments (1)
src/app/externalLinks.ts:8
PermalinkRoutingdeliberately leaves modified/non-leftmatrix:URIs to their external behavior, but this allowlist rejects every non-HTTP(S)/mailto scheme. Withopen_js_links_on_click(false)in the new opener plugin, Ctrl/Cmd- or middle-clicking amatrix:link has no Tauri opener path and can stay in the desktop webview instead of opening externally. Handle the Matrix/custom schemes with matching scoped permissions, or preserve the plugin/default handling for them.
return ["http:", "https:", "mailto:"].includes(url.protocol) ? url : null;
- Files reviewed: 16/17 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Addressed the suppressed custom-scheme finding from review #5227193215 in 3f321ea. Desktop external routing now includes the supported Matrix message-link schemes ( The same commit fixes a local-review finding about article-preview downloads: image filenames now derive their extension from the fetched image type rather than the webpage path. Validation: lint, typecheck, 4,267 unit tests, 163 browser tests, production build, cargo check --locked, and a fresh local Codex review with no actionable findings. Requesting review of the updated HEAD. |
There was a problem hiding this comment.
🔵 Needs a closer look
Fix external-image downloads and close the viewer when navigation changes only the URL fragment.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
src/features/room/urlPreviews/LinkedImageViewer.tsx:15
- This close effect only subscribes to
pathnameandsearch. A navigation that changes only the URL fragment will leavelinkedImageset and the lightbox open, even thoughuseLocation()exposeshashand the app preserves it as part of navigation state (for example insrc/app/Layout.tsx:960). Readlocation.hashhere as well so every URL navigation closes the viewer.
src/features/room/urlPreviews/LinkedImageViewer.tsx:61
- For an uncached direct link,
fullUrlis the third-party image URL, butImageLightboxdownloads plain images withfetch(img.fullUrl, { credentials: "omit" }). A cross-origin<img>can render without CORS while that fetch is blocked by most image hosts, so the existing Download action fails for otherwise viewable linked images. Please route this download through a same-origin/server proxy, or hide/replace the download action for direct external URLs and keep “Open in browser” as the fallback.
eventId: current.sourceUrl,
fullUrl: current.fullUrl,
externalUrl: current.sourceUrl,
- Files reviewed: 16/17 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Addressed both suppressed findings from review #5227362999 in 260b155:
The expanded tests also caught and fixed focus loss after Retry, which could prevent Escape from closing the viewer afterward. All checks passed: lint, typecheck, 4,267 unit tests, 166 browser tests, production build, and a fresh local Codex review with no actionable findings. Requesting a new review of HEAD 260b155. |
There was a problem hiding this comment.
🔵 Needs a closer look
Preserve article preview alt text and prevent video previews from opening as images.
Review details
Suppressed comments (2)
src/features/room/urlPreviews/UrlPreviewCard.tsx:126
UrlPreviewData.image.altis preserved by the cache and used on the card thumbnail, but this path drops it when constructingLinkedImage. For an article URL,isImageLinkis false, soLinkedImageViewersuppliesfilename: nullandImageLightboxexposes the enlarged image asalt="Image", losing the only meaningful accessible description. Carry the OG alt text through the linked-image/lightbox descriptor while keepingfilenamefor download naming.
setLinkedImage({
sourceUrl: props.url,
fullUrl,
...(img?.width !== undefined ? { width: img.width } : {}),
...(img?.height !== undefined ? { height: img.height } : {}),
src/features/room/urlPreviews/UrlPreviewCard.tsx:137
- The thumbnail handler treats
isVideo()previews as external, but this condition only checks the URL suffix. A video preview whose source URL ends in.jpg/.pngwill therefore open the full-screen image from its title while the thumbnail opens the video in the browser. Exclude video previews from the in-app title path so both controls navigate consistently.
event.altKey ||
!isImageLink(props.url)
- Files reviewed: 16/17 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Addressed both findings from review 5227526246 in 17e36c6:
The broader local review also caught and fixed thumbnail modified/middle-click navigation and article thumbnail download names. Preview thumbnails remain real links, and metadata survives filename selection so an article at a .jpg URL cannot name a cached PNG download. The independent local reviewer ran lint, typecheck, 4,268 unit tests, 170 browser tests, and the production build successfully on the final state, then reported no remaining actionable findings. Browser/test startup restrictions were handled with approved command-specific retries in normal Codex execution mode. The dedicated local review was also completed before this push. |
There was a problem hiding this comment.
🟡 Changes recommended
Cached metadata-only previews can misclassify image-looking URLs; preserve their type metadata before applying image-link interception.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 17/18 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
An unresolved moderate issue can render an article URL as an image when cached media is unavailable.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 18/19 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
Align the hero card DOM order with its visual order so keyboard focus follows the displayed sequence.
Review details
Suppressed comments (1)
src/features/room/urlPreviews/UrlPreviewCard.tsx:171
- On hero cards, the title anchor is first in the DOM but is rendered after the thumbnail via
order-2(the image isorder-1below). Because these are now two separate focusable links, keyboard focus proceeds title → image while the visual order is image → title, which makes navigation inconsistent for keyboard users. Keep the DOM and visual order aligned, for example by making the image first in the DOM and applying explicit flex orders for compact cards.
class={`min-w-0 flex-1 rounded no-underline hover:bg-surface-3 focus-visible:outline-hidden focus-visible:ring-2 focus-visible:ring-border-focus ${isHero() ? "order-2 w-full p-2" : "self-stretch content-center"}`}
- Files reviewed: 18/19 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Addressed the suppressed focus-order finding from review 5227994518 in fe023d3. Hero previews now display title then image, matching DOM and keyboard order; compact cards retain their title-then-thumbnail layout. Browser regressions verify visual rectangles and Tab order for both layouts. The local reviewer also caught a sibling shared-image failure case before push. Focus restoration now runs when the thumbnail is removed, covering a failure emitted by a different card sharing the same image. A two-card browser regression reproduced and verifies that fix. Both local reviews completed with no remaining actionable findings. The independent reviewer ran lint, typecheck, 4,270 unit tests, 175 browser tests, and the production build successfully using command-specific approved retries. |
There was a problem hiding this comment.
🔵 Needs a closer look
Pending preview metadata can misclassify article or video URLs ending in .jpg before their metadata settles.
Review details
Suppressed comments (1)
src/features/room/urlPreviews/LinkedImageViewer.tsx:36
peekPreview()only reads the settled cache; whilegetOrFetchPreview()is still in flight it returnsundefined(seepreviewCache.ts:146-168). This handler then treats an image-looking URL as a direct image before the pendingog:typeis known, so an article/video page ending in.jpgcan open the HTML page in the viewer instead of retaining external navigation. Track/await the in-flight preview for this click (and only apply the suffix fallback after it settles) so the metadata override is deterministic.
const metadata = peekPreview(anchor.href);
if (!isImageLink(anchor.href, metadata?.type)) return;
- Files reviewed: 18/19 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
For review 5228117852, retaining immediate click handling is deliberate; no code change for the suggested wait. The classifier is explicitly a URL-suffix heuristic when no settled metadata is available. Known article/video metadata overrides it. Previews can be disabled, unavailable, or still loading, so waiting for an in-flight request would make identical links behave differently based on network timing without establishing a general guarantee that the source is an image. It also delays the user interaction and can lose browser popup activation if the eventual result needs external navigation. The fallback is bounded and visible: a non-image response fails image loading in ImageLightbox, which presents an inline failure and retains the original source URL in Open in browser. It never navigates the app to the source HTML. The existing uncached-link browser regression verifies immediate opening and the external action; the load-failure regression verifies the failure/Retry path. Metadata-only, rejected cached media, and settled article/video overrides now have separate regressions. This tradeoff matches the request for immediate in-app image viewing and the project's interaction-latency requirement. The latest commit fe023d3 passes all local and CI checks. Please assess the implementation with this intentional best-effort metadata behavior in mind. |
There was a problem hiding this comment.
🔵 Needs a closer look
Unresolved moderate review comments remain.
Review details
Suppressed comments (3)
src/features/room/urlPreviews/LinkedImageViewer.tsx:36
peekPreviewis a settled-cache read, butUrlPreviewListstartsgetOrFetchPreviewasynchronously. If a user clicks an image-looking URL before that request settles,metadatais undefined and this handler intercepts it as an image even when the response will beog:type: articleorvideo.*, so the metadata override is not reliable. Please either await/reuse the in-flight preview before preventing navigation or fail open while metadata is unresolved, and cover the click-before-preview case.
const metadata = peekPreview(anchor.href);
if (!isImageLink(anchor.href, metadata?.type)) return;
src/features/room/urlPreviews/LinkedImageViewer.tsx:91
- This fallback is global rather than tied to the link that opened the viewer. If a linked image opened from a thread, pinned panel, or another virtualized timeline is recycled while the lightbox is open, closing it can focus the first main-room timeline scroller instead of the originating context. Capture the opener's nearest scroll/focus container (with an appropriate fallback) when opening so focus does not jump out of the thread or panel.
fallbackFocus={() =>
document.querySelector<HTMLElement>('[data-testid="timeline-scroller"]')
}
src/features/room/urlPreviews/UrlPreviewCard.tsx:183
- This cleanup belongs to the image
<Show>owner, so it also runs when the whole card is unmounted (for example during room navigation), not only when a shared image fails. If the thumbnail has focus then, it focusestitleLinkjust before both nodes are removed, so focus still ends onbodyinstead of a valid fallback. Gate the restoration on the image having actually failed (and the title link still being connected).
onCleanup(() => {
if (document.activeElement === imageLink) titleLink?.focus();
- Files reviewed: 18/19 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Addressed the two focus findings from review 5228222845 in 6ad643a. Both linked-image entry points now capture their opener's focusable ancestor chain. If the opener is recycled, modal close returns to the nearest connected ancestor, keeping a thread or panel as the focus context before falling back to the main timeline. Browser regressions cover recycled text and preview openers in a separate timeline. Thumbnail cleanup is now gated on an actual image failure and a connected title, so normal card unmounts do not move focus. Both local reviews found no functional issues. Their formatting finding was corrected and lint rerun successfully. Validation: lint, typecheck, 4,270 unit tests, 177 browser tests, production build. The independent reviewer ran the test suites using approved command-specific retries. The pending-metadata recommendation remains intentionally declined for the reasons in #688 (comment). URL classification is best-effort and immediate until metadata has settled; the viewer's load-failure surface retains Open in browser. The PR description now states that boundary explicitly. Please distinguish that documented behavior from a defect requiring asynchronous click handling. |
There was a problem hiding this comment.
🔵 Needs a closer look
Restore the play affordance for compact video previews.
Review details
Suppressed comments (1)
src/features/room/urlPreviews/UrlPreviewCard.tsx:225
- Compact video previews no longer render the play affordance. The previous branch showed the overlay for every
isVideo()card, but this condition now restricts it to hero cards, so small/portrait video thumbnails look like ordinary images even though their link is a video. Keep the overlay for compact cards as well.
<Show when={isVideo() && isHero()}>
- Files reviewed: 18/19 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Addressed the latest compact-video play-affordance finding in 5458e87 by restoring the original preview-card rendering, including its video overlay. The card now differs from main only by an anchor marker used by the image-click delegate. The same commit deliberately narrows the design to direct HTTPS raster-image URLs and the existing viewer. It removes the split thumbnail/title links, article-thumbnail viewing, preview metadata/cache routing, and custom download-name behavior. Image detection now has one explicit URL-suffix rule, independent of pending or cached OpenGraph metadata. A URL ending in an image suffix that returns HTML uses the viewer's existing failure state and external link. Ordinary article/video URLs preserve their original external navigation. The earlier context-focus fix remains: recycled openers return to a surviving focusable ancestor. The thumbnail cleanup path was removed with the split-link design. Existing attachment downloads remain unchanged; linked images provide Open in browser. Dedicated local review and a separate execution-capable reviewer completed. Lint, typecheck, 4,258 unit tests, 168 browser tests, and production build passed. Browser tests now intercept fixture media deterministically; a separate live check loaded the reported EVE JPG and verified viewport fit, external URL, and Escape. |
There was a problem hiding this comment.
🔵 Needs a closer look
Article or video previews with image-like URLs can incorrectly open in the image viewer instead of retaining external navigation.
Review details
Suppressed comments (1)
src/features/room/urlPreviews/UrlPreviewCard.tsx:137
- Every preview card gets this marker, but
LinkedImageVieweronly checksanchor.hrefwithisImageLinkand never seesprops.data.type. Anog:type: articleorvideo.*preview whose source URL happens to end in.jpg,.png, etc. will therefore open the page in the image viewer (and show the image-load error) instead of retaining its external navigation. Preserve the preview type in the marker/handler and exclude those preview types from in-app routing.
data-link-preview
- Files reviewed: 15/16 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Regarding review 5228580914 on 5458e87: no code change is planned for this finding because URL-suffix routing is the deliberate final scope of the simplified design, documented in the PR description and preceding review reply. Both message links and preview cards use the same isImageLink rule, regardless of whether preview metadata has arrived. Passing og:type from the card would make the same URL behave differently in message text versus its card, and depending on preview availability. Evidence: src/lib/imageLink.ts defines the HTTPS raster pathname rule in one place; LinkedImageViewer.tsx uses the original href and does not read preview metadata. Browser coverage verifies both entry points, successful loading, the image-error state with the original external URL retained, and ordinary article/video navigation. The actual reported EVE JPG loads and fits successfully in a separate live check. A misleading image-suffixed HTML URL intentionally shows the existing image-error surface with Open in browser available. This is a known, bounded limitation rather than a promise to infer response MIME type from OpenGraph data. The earlier compact-video affordance issue is fixed by restoring the original single-anchor card layout. All CI checks, including browser tests, passed on 5458e87. The latest overall review verdict is still Needs a closer look; this reply records the design rationale rather than claiming bot approval. |
Clicking a direct HTTPS image link in a message or its preview opens the existing full-screen image viewer on web and desktop. The viewer provides zoom, fit, Escape/focus restoration, and Open in browser. Ctrl/Cmd/Shift/Alt and middle clicks retain normal link navigation. Navigation closes the viewer; a recycled opener returns focus to its surviving context.
Image detection uses the URL pathname suffix (JPG/JPEG, PNG, GIF, WebP, AVIF), independently of preview metadata. The original image is requested only after the click. A failed or non-image response uses the existing error surface and retains the external link. HTTP images retain external navigation under the production CSP. Preview cards keep their original single-anchor structure and layout; ordinary article/video previews remain external links. Linked images use Open in browser rather than a cross-origin download fetch; attachment downloads keep their existing behavior.
Desktop external links use Tauri's scoped opener after in-app routing, fixing the silent desktop click. Validation: lint, typecheck, 4,258 unit tests, 168 browser tests, production build, and cargo check --locked. A separate live browser check loaded the reported EVE JPG, verified viewport fit, external navigation URL, and Escape. CI tests use intercepted fixture media and a mocked native bridge; the installed desktop executable has not been replaced by this PR build.