fix(comments): open image/PDF attachments in-app instead of trapping the app by h4yfans · Pull Request #829 · memrynote/memry

h4yfans · 2026-07-21T23:28:46Z

Fixes #799 (part of #795).

Problem

Clicking a comment's image or PDF attachment trapped the app. CommentAttachments rendered each attachment as a bare <a href="memry-file://…">, so clicking drove a main-frame navigation. decideFrameNavigation allowed memry-file: on the top frame, so Chromium replaced the React app with the protocol handler's raw bytes:

Image → a raw image document with no app chrome; Esc / click-outside did nothing (the app had unmounted), so only a restart recovered.
PDF → a blank/black frame (Electron has no inline PDF plugin) while the OS PDF app opened.

Fix

1. Comment attachment rendering (comment-attachments.tsx, new)

Images render an inline <img> thumbnail.
Clicking an image or PDF opens the existing in-app ImageViewer / PdfViewer inside a Dialog — closes with Esc, click-outside, or the ✕.
Other file types (docx, txt, …) open in the OS default app via window.open → setWindowOpenHandler → shell.openPath.
No navigable memry-file:// anchors anywhere.
PdfViewer is React.lazy-loaded — react-pdf/pdf.js crashes at module load outside a real DOM (DOMMatrix is not defined), and this keeps review-card's static import graph clean for jsdom tests (also defers a heavy dep until a PDF is actually opened).

2. Defense-in-depth navigation hardening (frame-navigation.ts, index.ts)

A main-frame memry-file: navigation now resolves to a new 'open-file' decision (subframes still 'allow'), routed through a shared openMemryFileInOs helper that is also reused by setWindowOpenHandler. Any future bare memry-file:// anchor opens in the OS instead of trapping the app.

Acceptance

✅ Comment image shows a thumbnail; clicking opens an in-app viewer that closes with Esc / click-outside.
✅ Comment PDF opens in the in-app PdfViewer (not a black frame / external app).

Notes for reviewers

Backward compatible: no DB / contract / IPC / vault-format / settings change. The attachment classifier tolerates older attachments with missing mimeType/type via a filename-extension fallback.
Comment attachments have no fileId (they live in a per-note attachments/<noteId>/ folder, not the indexed vault-files table), so the tab-based file-page opener does not apply — the viewers are reused directly in a Dialog.

Verification

frame-navigation 25 ✓ · comment-attachments 11 ✓ · full review dir 46 ✓ · main lib 195 ✓
typecheck (web + node) ✓ · IPC/contract/architecture checks ✓ · eslint 0 errors · i18n:check ✓ · docs:impact --strict covered · docs:build ✓

Manual smoke pending on a packaged/dev run: comment with an image, a PDF, and a .txt.

…the app Comment attachments rendered as a bare <a href="memry-file://…">. Clicking drove a main-frame navigation that decideFrameNavigation allowed for the memry-file scheme, replacing the React app with the protocol handler's raw bytes — images trapped the app (only a restart recovered) and PDFs rendered black while the OS PDF app opened (issue #799). - New comment-attachments.tsx: images show inline <img> thumbnails; clicking an image or PDF opens the existing in-app ImageViewer/PdfViewer in a Dialog (Esc / click-outside / X close). Non-viewable files open in the OS default app via window.open -> shell.openPath. No navigable memry-file anchors. PdfViewer is lazy-loaded (react-pdf crashes jsdom at module load). - Defense-in-depth: main-frame memry-file navigations now resolve to a new 'open-file' decision (subframes still allow), routed through a shared openMemryFileInOs helper reused by setWindowOpenHandler. No DB/contract/IPC/vault-format change; fully backward-compatible.

vercel · 2026-07-21T23:28:53Z

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments

Project	Deployment	Actions	Updated (UTC)
memry-docs	Ignored	Preview	Jul 22, 2026 12:24am
memrynote-landing	Ignored	Preview	Jul 22, 2026 12:24am

github-actions · 2026-07-21T23:29:17Z

React Doctor found no new issues. 🎉

_{Reviewed by React Doctor for commit 5808108.}

Copilot

Pull request overview

This PR fixes a UX-breaking bug where clicking comment image/PDF attachments triggered a main-frame navigation to memry-file://…, unmounting the React app and “trapping” users in a raw image/blank PDF document. It introduces in-app viewers for comment attachments and hardens main-frame navigation handling as defense-in-depth.

Changes:

Replace comment attachment rendering with inline image thumbnails and an in-app Dialog + ImageViewer / lazy PdfViewer for image/PDF attachments.
Route non-viewable attachments through window.open → Electron setWindowOpenHandler → allowlisted shell.openPath (OS default app).
Harden navigation policy so main-frame memry-file: navigations become an explicit 'open-file' decision and are routed through a shared allowlist-checked helper.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
packages/i18n/src/locales/en/notes.json	Adds ARIA label strings for opening image/attachment actions in comment UI.
apps/docs/src/user-guide/notes/editing.md	Documents the new comment attachment behavior (thumbnail + in-app viewers; OS open for other types).
apps/desktop/src/renderer/src/components/note/review/review-card.tsx	Removes the old inline anchor-based attachments renderer and wires in the new component.
apps/desktop/src/renderer/src/components/note/review/comment-attachments.tsx	New attachment renderer: thumbnails + dialog viewers for image/PDF; OS open for other file types; no navigable `memry-file://` anchors.
apps/desktop/src/renderer/src/components/note/review/comment-attachments.test.tsx	Adds unit tests for classification and ensuring correct viewer/OS routing behavior.
apps/desktop/src/main/lib/frame-navigation.ts	Introduces `'open-file'` decision for main-frame `memry-file:` navigations to prevent SPA replacement/trap.
apps/desktop/src/main/lib/frame-navigation.test.ts	Updates tests to assert main-frame `memry-file:` becomes `'open-file'` while subframes remain allowed.
apps/desktop/src/main/index.ts	Adds `openMemryFileInOs` helper (shared allowlist logic) and wires it into both the frame navigation guard and `setWindowOpenHandler`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

codecov · 2026-07-21T23:44:14Z

Codecov Report

❌ Patch coverage is 96.36364% with 2 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
apps/desktop/src/main/index.ts	92.85%	1 Missing ⚠️
...src/components/note/review/comment-attachments.tsx	97.50%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

…mpact cards Image and file attachments shared a wrapping flex row, so a PDF pill stretched to the image thumbnail's height. Stack attachments one per line and render non-image files as a compact bordered card (file icon + name + size) instead of a full-height background pill.

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

+  it('hands a non-viewable file to the OS via window.open — no in-app viewer, no trap', () => {
+    const openSpy = vi.spyOn(window, 'open').mockReturnValue(null)
+    const path = 'memry-file://local/vault/attachments/n1/notes.docx'
+    render(<CommentAttachments mark={markWith([att({ name: 'notes.docx', path })])} />)
+    fireEvent.click(screen.getByRole('button'))
+    expect(openSpy).toHaveBeenCalledWith(path, '_blank', 'noopener,noreferrer')
+    expect(screen.queryByTestId('image-viewer')).toBeNull()


Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

.critic-review-content sets pointer-events: none and only re-enabled it for anchors (the old attachments were <a> links). The new attachment component uses <button>, so clicks fell through to the expand-toggle overlay and the viewer never opened. Re-enable pointer-events for buttons too.

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

+  // App-controlled local scheme. Inline resource loads (<img>/<audio>/<video>,
+  // react-pdf fetch) never fire will-frame-navigate, so a memry-file event here
+  // is a real navigation. On the main frame that would replace the SPA document
+  // with the protocol handler's raw file bytes and trap the app (no in-app back
+  // path) — hand the file to the OS instead. Subframes may still load it in-place


h4yfans added 2 commits July 22, 2026 02:27

docs(notes): document comment attachment thumbnails and in-app viewers

db2e70d

Copilot AI review requested due to automatic review settings July 21, 2026 23:28

github-actions Bot added bug Something isn't working documentation Improvements or additions to documentation test labels Jul 21, 2026

Copilot started reviewing on behalf of h4yfans July 21, 2026 23:29 View session

Copilot AI reviewed Jul 21, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings July 21, 2026 23:56

Copilot started reviewing on behalf of h4yfans July 21, 2026 23:58 View session

Copilot AI reviewed Jul 22, 2026

View reviewed changes

test(comments): restore spies after each test to prevent mock leakage

5740bf6

Copilot AI review requested due to automatic review settings July 22, 2026 00:04

Copilot started reviewing on behalf of h4yfans July 22, 2026 00:04 View session

Copilot AI reviewed Jul 22, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings July 22, 2026 00:23

Copilot started reviewing on behalf of h4yfans July 22, 2026 00:24 View session

h4yfans marked this pull request as ready for review July 22, 2026 00:26

h4yfans merged commit cabd3ff into main Jul 22, 2026
18 of 19 checks passed

h4yfans deleted the comment-attachment-inapp-viewers branch July 22, 2026 00:26

Copilot AI reviewed Jul 22, 2026

View reviewed changes

h4yfans mentioned this pull request Jul 27, 2026

fix(notes): restore page navigation on inline PDF embeds #902

Merged

Navigation Menu

Uh oh!

fix(comments): open image/PDF attachments in-app instead of trapping the app - #829

fix(comments): open image/PDF attachments in-app instead of trapping the app#829
h4yfans merged 5 commits into
mainfrom
comment-attachment-inapp-viewers

h4yfans commented Jul 21, 2026

Uh oh!

vercel Bot commented Jul 21, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 21, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

codecov Bot commented Jul 21, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

h4yfans commented Jul 21, 2026

Problem

Fix

Acceptance

Notes for reviewers

Verification

Uh oh!

vercel Bot commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

codecov Bot commented Jul 21, 2026

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

vercel Bot commented Jul 21, 2026 •

edited

Loading

github-actions Bot commented Jul 21, 2026 •

edited

Loading