Replace Pdf loader spec with behavior-first contract tests

[Copilot]

Description

This PR replaces catalog/app/components/Preview/loaders/Pdf.spec.ts with a stronger, behavior-oriented suite targeting Pdf.js runtime contracts through Loader. It removes tautological tests and increases coverage where regressions are most likely (request contract + 403 mapping split).

• Behavior over implementation detail

  • Replaced inline endsWith('.pptx') ? 'pptx' : 'pdf' style tests with loader-driven assertions.
  • Verifies logicalKey precedence by observing real request behavior (input=pptx), not re-deriving branch logic in the test.

• Request contract assertions

  • Validates /thumbnail request shape via new URL(...) and explicit query param checks (url, input, size, countPages), instead of substring matching.

• Error-path coverage

  • Preserves glacier 403 → PreviewError.Archived.
  • Adds non-glacier 403 → PreviewError.Forbidden.
  • Preserves unexpected-error rethrow behavior.

• Loader wiring + async cardinality

  • Asserts one async kickoff per render (pending cardinality / call counts).
  • Asserts useErrorHandling is called with { handle, retry: data.fetch } and that children receive the wrapped value.

• CI/runtime compatibility follow-up

  • Adjusts the blob assertion to avoid cross-runtime Blob realm mismatch in CI while still validating the returned blob payload shape and non-empty content.

const requestUrl = new URL(fetchMock.mock.calls[0][0])
expect(requestUrl.pathname).toBe('/thumbnail')
expect(requestUrl.searchParams.get('input')).toBe('pdf')
expect(requestUrl.searchParams.get('countPages')).toBe('true')

TODO

• Unit tests
• Security: Confirm that this change meets security best practices and does not violate the security model
• Open and Embed: Confirm that this change doesn't break Open variant and Embed widget
• Documentation
  • run optipng on any new PNGs
  • Python: Run build.py for new docstrings
  • JavaScript: basic explanation and screenshot of new features
  • Markdown somewhere in docs/**/*.md that explains the feature to end users (said .md files should be linked from SUMMARY.md so they appear on https://docs.quilt.bio)
  • Markdown docs for developers
• Changelog entry (skip if change is not significant to end users, e.g. docs only)

Original prompt

Create a new pull request against master in quiltdata/quilt that replaces catalog/app/components/Preview/loaders/Pdf.spec.ts with a stronger version of the spec.

Context:

• There is already an open PR (feat(catalog): Frontend companion to the preview/lambda feature PR #4936 #4937) that adds catalog/app/components/Preview/loaders/Pdf.spec.ts.
• The goal is to open a separate PR containing an improved spec file, not to modify the existing PR.
• The PR body should explain why the new spec is better.

Repository context discovered from the current codebase:

• The production loader is implemented in catalog/app/components/Preview/loaders/Pdf.js.
• detect is utils.extIn(['.pdf', '.pptx']).
• loadPdf signs the handle, derives type from (handle.logicalKey || handle.key).toLowerCase().endsWith('.pptx') ? 'pptx' : 'pdf', fetches /thumbnail with url, input, size, and countPages, parses X-Quilt-Info.page_count, and maps HTTPError 403s to either PreviewError.Archived({ handle }) when the glacier regex matches or PreviewError.Forbidden({ handle }) otherwise.
• Loader calls AWS.Signer.useS3Signer(), Data.use(loadPdf, { sign, handle }), and wraps the result with utils.useErrorHandling(data.result, { handle, retry: data.fetch }).

Problems in the currently proposed spec from PR #4937:

• It includes tautological tests that reimplement the branch logic inline instead of exercising the loader (determines type as pptx for .pptx keys, determines type as pdf for .pdf keys, uses logicalKey over key when available for type detection).
• It verifies fetch request construction only via stringContaining fragments instead of asserting the full request contract.
• It does not test the non-glacier 403 -> PreviewError.Forbidden path even though the production code explicitly supports it.
• It does not assert cardinality for async work (pending[0] only) or direct invocation of useErrorHandling.
• It should favor behavioral tests through Loader over tests that restate implementation details.

What to change:

1. Replace the current catalog/app/components/Preview/loaders/Pdf.spec.ts with a stronger test suite.
2. Keep using Vitest and the existing mocking approach where practical.
3. Preserve valuable coverage:
   • detect accepts .pdf and .pptx case-insensitively and rejects non-matching names.
   • happy path for PDF thumbnail loading
   • logicalKey-driven PPTX behavior
   • glacier 403 mapping to Archived
   • unexpected error rethrow behavior
4. Improve the suite by adding/adjusting tests to cover:
   • exact request contract parsing using new URL(...) rather than only substring checks
   • explicit assertion that only one async load was kicked off when appropriate (pending length / call counts)
   • direct assertion that useErrorHandling is called with { handle, retry: data.fetch }
   • non-glacier 403 mapping to PreviewError.Forbidden
   • logicalKey precedence via actual loader behavior, not via standalone inline expressions
5. Remove tautological tests that just duplicate endsWith('.pptx') ? 'pptx' : 'pdf' outside the production code.
6. Keep the spec focused on observable behavior and stable contracts.

Expectations for the resulting PR:

• Only include the necessary file changes for the improved PDF spec.
• Open a PR.
• Write a clear PR title.
• In the PR body, explain why this spec is better, specifically calling out:
  • behavior-over-implementation testing
  • stronger request-contract assertions
  • better error-path coverage, especially Forbidden vs Archived
  • removal of self-fulfilling/tautological tests

If you need to inspect the existing file path, use catalog/app/components/Preview/loaders/Pdf.spec.ts and catalog/app/components/Preview/loaders/Pdf.js.

The following is the prior conversation context from the user's chat exploration (may be truncated):

User: Perform an adversarial review of my new PDF spec and provide suggestions to improve it.

Assistant: This spec is a good start but too implementation-coupled, and an adversarial review would push for fewer tautological tests, stronger behavioral assertions, and better failure-shape coverage.

Main concerns

1. Several tests duplicate the implementation instead of validating behavior

These tests are weak because they restate the branching logic inline:

• determines type as pptx for .pptx keys
• determines type as pdf for .pdf keys
• uses logicalKey over key when available for type detection

They don’t exercise Loader or a real exported helper; they just compute:

const type = key.toLowerCase().endsWith('.pptx') ? 'pptx' : 'pdf'

That means they can pass even if the production code regresses elsewhere.

Suggestion: remove or replace them with tests that verify observable behavior through Loader, e.g. that the request includes input=pptx or input=pdf for representative handles.

---

2. The happy-path test does not validate enough of the returned payload

In `fetches thumbnail with correct parameters for ...

This pull request was created from Copilot chat.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.21%. Comparing base (8eec5f4) to head (50f2f9b).

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #4948       +/-   ##
===========================================
+ Coverage   46.50%   91.21%   +44.70%     
===========================================
  Files         832       17      -815     
  Lines       34090      660    -33430     
  Branches     5833        0     -5833     
===========================================
- Hits        15854      602    -15252     
+ Misses      16237       58    -16179     
+ Partials     1999        0     -1999     

Flag	Coverage Δ
api-python	?
catalog	?
lambda	83.27% <ø> (-13.36%)	⬇️
py-shared	98.02% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Austin-s-h]

@copilot consider the current PR branch target and perform a review of before and after your spec redesign. This should result in comparison of the spec in both branches.

[Copilot]

@copilot consider the current PR branch target and perform a review of before and after your spec redesign. This should result in comparison of the spec in both branches.

Compared against the PR target branch (feat/preview-frontend) directly (origin/feat/preview-frontend vs copilot/replace-pdf-spec-with-improved-version) for catalog/app/components/Preview/loaders/Pdf.spec.ts.

Before (target branch): 11 tests, including several implementation-rederived checks (manual extension ternaries, substring URL assertions).
After (this branch): 7 behavior-first tests focused on loader contracts:

• request contract checked via parsed URL query params
• logicalKey precedence validated through actual input=pptx request behavior
• explicit 403 split (Archived for glacier, Forbidden otherwise)
• loader wiring/cardinality checks (pending, useErrorHandling, wrapped child value)

I also fixed a CI portability issue discovered during this branch-aware review (Blob cross-runtime instanceof assertion) in commit 9dd369ed.