IN-PROGRESS: Show PDF resources in the browser's inline viewer#1893
IN-PROGRESS: Show PDF resources in the browser's inline viewer#1893maebeale wants to merge 1 commit into
Conversation
| <%= render "assets/display_assets", | ||
| resource: resource, file: resource.display_image, | ||
| variant: :hero, link: true %> | ||
| <% if pages > 1 %> |
There was a problem hiding this comment.
🤖 From Claude: Inline embed is gated on pages > 1: single-page PDFs keep the crisp first-page image, and if page count is unknown (poppler missing or not yet analyzed) we fall back to the image rather than embedding. The display_assets render inside <object> is the fallback for browsers that won't render inline PDFs (notably mobile Safari/Chrome).
| # when poppler isn't installed. | ||
| class PdfPageCountAnalyzer < ActiveStorage::Analyzer | ||
| class << self | ||
| def accept?(blob) |
There was a problem hiding this comment.
🤖 From Claude: accept? returns false when pdfinfo is absent, so the analyzer is a no-op (no pages metadata) rather than raising — the view then degrades to the first-page preview. Mirrors how HighResPopplerPdfPreviewer guards on pdftoppm.
| # poppler-utils provides pdftoppm/pdfinfo, used by the PDF previewer and the | ||
| # PDF page-count analyzer (mirrors the production runtime dependency). | ||
| - name: Install poppler-utils | ||
| run: sudo apt-get update && sudo apt-get install -y poppler-utils |
There was a problem hiding this comment.
🤖 From Claude: poppler is already a prod runtime dependency (the PDF previewer shells out to pdftoppm); installing it here lets the new PDF specs actually run instead of self-skipping.
maebeale
force-pushed
the
maebeale/prod-staging-resource-mismatch
branch
from
c3d0955 to
10a0ae9
Compare
maebeale
changed the title
|
I see the pdf viewer for multi page is just using the built in browser feature. That's a good thing since all major browsers support it. In that case, my vote would be to completely remove the code around counting the number of pages. If This would work well on the Resource show as well. |
maebeale
changed the title
Callout resource pages and the resource detail page previewed only a PDF's first page as an image, so multi-page handouts looked like one-pagers. Render PDFs in the browser's built-in inline viewer instead (scrollable, all pages — every major browser ships one), falling back to the first-page/image preview for the rare client that can't. Non-PDF resources keep the existing preview. Extracted the PDF-or-preview choice into assets/_resource_display so the callout viewer and resource show page share it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
maebeale
force-pushed
the
maebeale/prod-staging-resource-mismatch
branch
from
10a0ae9 to
0aa0cb6
Compare
maebeale
changed the title
| can't. Anything else (images, etc.) renders through the shared asset | ||
| pipeline. `resource` is a decorated Resource. %> | ||
| <% display = resource.display_image %> | ||
| <% if display.respond_to?(:attached?) && display.attached? && display.content_type == "application/pdf" %> |
There was a problem hiding this comment.
🤖 From Claude: The PDF check keys off display_image (the resolved hero: primary_asset → downloadable_asset → …), not the downloadable file directly — so a resource with a primary image and a downloadable PDF still shows its image here, and only resources whose hero actually is a PDF get the inline viewer. respond_to?(:attached?) guards against the symbol/string defaults display_image can return.
There was a problem hiding this comment.
Pull request overview
This PR updates resource/callout rendering so PDFs can be displayed inline via the browser’s built-in PDF viewer (instead of only showing a first-page preview), and removes the prior hardcoded “single-page PDF titles” logic. It also adjusts request specs around callouts and bumps the checkout Action version in CI.
Changes:
- Add a shared
assets/_resource_displaypartial that renders PDFs via an inline<object>viewer and falls back to the existing asset pipeline for non-PDFs. - Remove
Resource#single_page_pdf?and its associated specs / “download for all pages” prompt in registration resource specs. - Update callout request specs to assert PDF vs non-PDF rendering, and bump
actions/checkoutin CI.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| spec/requests/events/registrations_spec.rb | Removes expectations about the “download for all pages” prompt on multi-page resources. |
| spec/requests/events/registration_ticket_callouts_spec.rb | Adds coverage asserting inline-PDF rendering for PDF resources and no inline-PDF markup for non-PDF resources. |
| spec/models/resource_spec.rb | Removes tests for the deleted #single_page_pdf? behavior. |
| app/views/resources/show.html.erb | Switches the hero asset rendering to the shared assets/resource_display partial. |
| app/views/events/callouts/_resource_body.html.erb | Removes the “download for all pages” prompt and uses the shared assets/resource_display partial. |
| app/views/assets/_resource_display.html.erb | New partial that renders PDFs inline via <object> and uses the existing display_assets pipeline otherwise. |
| app/models/resource.rb | Deletes the hardcoded single-page title list and #single_page_pdf?. |
| AGENTS.md | Updates/extends architecture reference entries (counts/models/concerns). |
| .github/workflows/ci.yml | Bumps actions/checkout major version in the CI workflow. |
| <% display = resource.display_image %> | ||
| <% if display.respond_to?(:attached?) && display.attached? && display.content_type == "application/pdf" %> |
🤖 PR, suggested 👤 review level: 📖 Read — light-logic: a shared view partial branching on PDF vs. not, plus removal of the earlier page-count machinery
Callout resource pages and the resource detail page previewed only a PDF's first page as an image, so multi-page handouts looked like one-pagers.
assets/_resource_display, shared by the callout resource viewer and the resource show page.Per review feedback, dropped the page-counting approach entirely (the
PdfPageCountAnalyzer+metadata["pages"]badge, thepoppler-utilsCI step, thepdfs:backfill_page_countstask, and the hardcodedsingle_page_pdf?) — the browser viewer shows every page, so none of it is needed. No backfill or deploy step required now.