Skip to content

Preserve page deletions when restoring the remaining page order - #21972

Open
yunfeizhu wants to merge 1 commit into
mozilla:masterfrom
yunfeizhu:fix/preserve-deleted-pages-after-reorder
Open

yunfeizhu wants to merge 1 commit into
mozilla:masterfrom
yunfeizhu:fix/preserve-deleted-pages-after-reorder

Conversation

@yunfeizhu

@yunfeizhu yunfeizhu commented Sep 17, 2026 •

Copy link
Copy Markdown

Fixes #21971.

After deleting trailing pages and moving the remaining pages back to their original order, PagesMapper clears the mapping even though the document is shorter. The viewer then treats the document as unchanged and downloads the original PDF, bringing the deleted pages back.

Track the original page count and only clear the mapping when both the count and order match the original document.

Add regression coverage to the existing Reorganize Pages View > Save a pdf integration suite. The tests delete one or three trailing pages through the sidebar, restore the remaining pages' order, click the main Save button, and verify the saved page selection and order. The original-download path is also captured so the regression fails an assertion instead of timing out.

Validation

  • Chrome 152 and Firefox Nightly 158: all 3 save integration tests pass, including the 2 new regressions, using a TESTING=true generic build.
  • With the original PagesMapper implementation, both new tests fail in both browsers because saving takes the original-download path. The existing save test still passes.
  • TESTING=true npx gulp generic and npx gulp lint pass.

The full test suite has not been run locally. The regression tests follow the existing integration-test convention of checking the save payload, since actual saving is disabled in testing builds.

@Snuffleupagus

Snuffleupagus commented Sep 17, 2026 •

Copy link
Copy Markdown
Collaborator

Shouldn't this use an integration-test, similar to all the existing split/merge tests, rather than a unit-test?

See also https://github.com/mozilla/pdf.js/wiki/Squashing-Commits

@yunfeizhu
yunfeizhu force-pushed the fix/preserve-deleted-pages-after-reorder branch from 5df0b68 to d96eab4 Compare September 17, 2026 08:53
@yunfeizhu

yunfeizhu commented Sep 17, 2026 •

Copy link
Copy Markdown
Author

Thanks for the review. I've moved the regression coverage into the existing Reorganize Pages View > Save a pdf integration suite and squashed the changes into a single commit.

The new cases cover deleting one or three trailing pages, restoring the remaining page order, and checking the Save payload. Both fail without the fix and pass with it in Chrome and Firefox. The existing save test, generic build, and lint also pass.

@yunfeizhu
yunfeizhu force-pushed the fix/preserve-deleted-pages-after-reorder branch from d96eab4 to 2e7abc0 Compare September 17, 2026 08:56
Comment thread test/integration/reorganize_pages_spec.mjs Outdated
@yunfeizhu
yunfeizhu force-pushed the fix/preserve-deleted-pages-after-reorder branch from 2e7abc0 to 808065a Compare September 17, 2026 09:23
@codecov-commenter

codecov-commenter commented Sep 17, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.31%. Comparing base (ee470d5) to head (808065a).
⚠️ Report is 84 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #21972      +/-   ##
==========================================
+ Coverage   90.30%   90.31%   +0.01%     
==========================================
  Files         269      269              
  Lines       67537    67539       +2     
==========================================
+ Hits        60989    60999      +10     
+ Misses       6548     6540       -8     
Flag Coverage Δ
browsertest 66.17% <50.00%> (+<0.01%) ⬆️
fonttest 8.92% <ø> (ø)
integrationtest 69.36% <100.00%> (+0.01%) ⬆️
unittest 59.50% <100.00%> (-0.01%) ⬇️
unittestcli 58.00% <100.00%> (+0.01%) ⬆️

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.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Deleted trailing pages return when downloading after restoring page order

4 participants