Skip to content

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

Merged
h4yfans merged 1 commit into
mainfrom
pdf-embed-page-nav
Jul 27, 2026
Merged

fix(notes): restore page navigation on inline PDF embeds#902
h4yfans merged 1 commit into
mainfrom
pdf-embed-page-nav

Conversation

@h4yfans

@h4yfans h4yfans commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator

Summary

A user on v2026-07-19.2 reported dropping a PDF into a note and getting the first page with no way to reach the others. That is a regression from #763: the chromeless embed rework pinned the inline preview to pageNumber={1} and deleted the page controls along with the header, footer and thumbnail sidebar. git tag --contains 388daf447 confirms it went out in v2026-07-17 through v2026-07-19.2, and file-block.tsx has not been touched since, so it is still live on main.

This restores paging without giving up the chrome-free look:

  • numPages / currentPage are tracked again and the page renders currentPage.
  • The controls are a bottom-centered [‹] 1 / 12 [›] pill that appears only on hover or keyboard focus, and only renders when numPages > 1 — single-page embeds stay exactly as they are today.
  • The current page is view state, not a block prop. The <!-- file:{...} --> marker is untouched and serialization is unchanged, so existing notes stay byte-identical on re-save. No migration or compat concern.

Note that the full-file viewer (components/viewers/pdf-viewer.tsx, also used by comment attachments since #829) is a separate component and always kept its arrows, counter and sidebar. Only the note embed lost them.

Reviewer notes

Three constraints shaped the placement, all previously learned the hard way on this card:

  • The pill sits at z-10. react-pdf's .react-pdf__Page__textContent is z-index: 2 and silently swallows clicks on anything below it.
  • It is centered with pointer-events-none absolute inset-x-0 bottom-2 flex justify-center plus a pointer-events-auto inner pill, rather than start-1/2 + rtl:translate-x-1/2. This keeps it RTL-safe without a translate, and leaves the page selectable underneath. Chevron glyphs flip via rtl:rotate-180.
  • Bottom-center is the only free spot on the card: the resize brackets own both bottom corners (-bottom-1 -start/-end-1) and the alignment toolbar owns top-2 end-2.

Release note

Multi-page PDFs embedded in a note can be paged through again — hover the embed for page controls.

Test plan

  • file-block unit tests — 7 passed, 0 failed (2 new: paging through a multi-page PDF, and single-page PDFs staying chromeless). Both were red before the fix on the missing previousPage control.
  • content-area + viewers renderer suites — 408 passed, 0 failed, 6 skipped.
  • pnpm --filter @memry/desktop typecheck:web — clean.
  • eslint --no-cache on the changed component — 0 errors, 0 warnings.
  • pnpm --filter @memry/desktop i18n:check — passed (new previousPage / nextPage keys).
  • pnpm docs:impact --base origin/main --strict — no docs-relevant changes.
  • prettier --check, git diff --check — clean.

Not covered: no E2E. The existing tests/e2e/pdf-embed-resize.e2e.ts fixture is a single-page PDF, and a paging test needs a new multi-page fixture with a byte-correct xref or pdfjs refuses to load it. Happy to add one if it is wanted before merge.

The chromeless embed rework (#763) pinned the inline PDF preview to
`pageNumber={1}` and deleted the page controls along with the rest of the
chrome. That shipped in v2026-07-19.2, where it reads as a bug: a dropped
multi-page PDF renders its first page with no way to reach the others.

Bring paging back without giving up the chrome-free look. `numPages` and
`currentPage` are tracked again and the page renders `currentPage`; the
controls are a bottom-centered pill that only appears on hover or focus,
and only exists when the document actually has more than one page, so
single-page embeds stay exactly as they are.

The current page is view state, not a block prop, so the `<!-- file:{...} -->`
marker is untouched and existing notes stay byte-identical on re-save.

Placement dodges the two overlays already on the card: the resize brackets
at the bottom corners and the alignment toolbar at the top-inline-end. The
pill is centred with `inset-x-0` + `justify-center` rather than a translate
so it is RTL-safe, the chevrons flip with `rtl:rotate-180`, and it sits at
`z-10` to clear react-pdf's text layer (`z-index: 2`), which otherwise
swallows the clicks.
Copilot AI review requested due to automatic review settings July 27, 2026 17:00

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions github-actions Bot added bug Something isn't working test labels Jul 27, 2026
@github-actions

Copy link
Copy Markdown

React Doctor found no new issues. 🎉

Reviewed by React Doctor for commit 3b4ca12.

@codecov

codecov Bot commented Jul 27, 2026

Copy link
Copy Markdown

Codecov Report

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

Files with missing lines Patch % Lines
...er/src/components/note/content-area/file-block.tsx 81.81% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

@h4yfans
h4yfans marked this pull request as ready for review July 27, 2026 21:20
@h4yfans
h4yfans merged commit 3b930fd into main Jul 27, 2026
17 of 18 checks passed
@h4yfans
h4yfans deleted the pdf-embed-page-nav branch July 27, 2026 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants