Feat/reader go to page by LindaMukundwa · Pull Request #1294 · stumpapp/stump

LindaMukundwa · 2026-07-20T05:01:38Z

Description

Issue #814 - feat(reader): go-to-page control, and fix PDFium binding reuse & thread-safety

Summary

This PR contains two independent changes that came out of getting the reader working end-to-end during local setup:

Feature: a "go to page" control in the image-based reader for quick position recovery.
Demo: https://imgur.com/a/fg4fzLz
Fix: corrects a PDFium binding bug (every PDF page after the first failed) and a latent thread-safety issue in the PDF renderer.

They're bundled because the PDF fix was a prerequisite to exercising the reader locally. Happy to split into two PRs if preferred and any other changes are requested. I am in a class learning to contribute to open-source projects, while being AI-native and critical of the output; therefore, AI assistance from Claude was used in part for this contribution. This is my first contribution to any open-source project ever, so I appreciate any advice and constructive criticism. Thank you!

1. Feature: "Go to page" control

What & why??

This was already an open issue. In paged mode, the footer's position indicator (e.g., "4-5 of 42") is now a clickable trigger that opens a small popover with a number input, letting the reader jump directly to any page. This is a manual-recovery affordance: if the reading position is ever lost, the user can type the page they remember and return to it immediately.
Scope

Image-based reader, paged mode only. Continuous-scroll modes keep the plain-text indicator, since their page-change path doesn't sync to the URL the same way; this was deferred intentionally.
EPUB reader unaffected (it's location/CFI-based with its own TOC + bookmarks).

How it works

The new GoToPage component is presentation-only: it takes currentPage, totalPages, and an onSubmit callback. It owns no reader state and performs no navigation itself.
In ReaderFooter, onSubmit is bound to the existing setCurrentPage handler, so a jump goes through the same state + URL (?page=N) + progress-write path as every other page change. No new navigation logic.
Input is validated and clamped to 1..totalPages; non-numeric input is ignored. Submits on click or Enter.

Files

packages/browser/.../container/GoToPage.tsx : new component + exported clampPage helper.
packages/browser/.../container/tests/GoToPage.test.tsx: unit tests.
packages/browser/.../container/ReaderFooter.tsx : wires the control in (paged mode), reusing the existing page-range label.

Testing

0 unit tests: clamp above/below range, empty book, non-numeric rejection, click-submit, Enter-submit, trigger rendering.
yarn check-types + eslint clean on all changed files.
Manually verified in browser: paged mode → click indicator → enter page → reader jumps, URL updates to ?page=N, progress persists.

2. Fix: PDFium binding reuse + thread-safety ( Already fixed, can be ignored )

The Problem (three issues, surfaced in order)

PdfConfigurationError : pdfium_path was unset and no system PDFium existed. (Resolved locally by installing libpdfium.dylib and pointing config at it, environment/config, no code change.)
PdfiumLibraryBindingsAlreadyInitialized : a real code bug. pdfium-render stores its loaded library in a process-global cell that can only be initialized once, but renderer() called bind_to_library() on every render, so the second page ever viewed failed. pdf_prerender_range = 5 made it fail instantly by rendering several pages concurrently.
Latent thread-safety: PDFium's native library isn't thread-safe, but renders run across multiple spawn_blocking threads.

Fix: core/src/filesystem/media/format/pdf.rs

Added a process-global PDFIUM_LOCK: Mutex<()> that serializes all PDFium access.
Rewrote renderer() to bind once and reuse existing bindings afterward - on PdfiumLibraryBindingsAlreadyInitialized it falls back to Pdfium::default(), which the crate documents as reusing already-initialized bindings. It now returns a MutexGuard held for the whole render.
Updated the three call sites (get_page_count, page render, and the file converter) to keep the guard alive via let (_guard, pdfium) = ….

Testing

Verified locally: PDFs now render past the first page, including with pdf_prerender_range = 5 (previously an instant failure); no binding-reinit errors under concurrent prerendering.

Setup note for reviewers/testers
To exercise PDF rendering you need a PDFium binary and pdfium_path set to it (macOS: libpdfium.dylib). (Possible follow-up: document this in docs/.../installation/source.mdx.)

Screenshots

https://imgur.com/a/fg4fzLz

Ready?

Please read each item and check the boxes:

[☑] I read the contributing guidelines
[☑] I searched for existing issues or pull requests that may be related to my contribution
[☑] This PR is based into nightly and not main
[☑] I added tests and/or documentation for my changes if applicable

Stump Contributor License Agreement

By contributing to Stump, you agree that your contributions will be licensed under the following licenses (where applicable):

The expo application is licensed under GPL-3.0
All other code in the repository is licensed under MIT License

aaronleopold · 2026-07-20T20:39:35Z

Can you rebase your changes to have nightly as the base? It looks like you have extra commits in the diff that should not be here

…e for the changes. Current version is paged only and working on embedding into footer. This changes causes little risk to app because nothing imports the component yet however the unit tests have been done and can be found. Still yet to test the manual in-browser usage but debug logs seem good.

…l in the image-based reader so that you can get back to where you need easily

LindaMukundwa · 2026-07-21T19:03:34Z

Hello! Thank you for your response. My apologies; I branched from the old v0.1.5 commit, which is 23 commits behind nightly. My c3468bd PDF commit already implements the exact fix that nightly already merged in [#1209] and therefore is a redundant commit. I believe I should have fixed these conflicts, but please feel free to let me know anything else that may be needed.

LindaMukundwa · 2026-08-03T02:27:16Z

Hello! I just wanted to check that and see if there are any changes you would like me to make to this request.

aaronleopold · 2026-08-03T02:50:06Z

Hey! Sorry I haven't had a moment to take a look yet, I needed to focus on some other project management tasks first to release 0.1.6 and clean up the PR queue. I'll give it a review sometime during the week

LindaMukundwa · 2026-08-03T03:25:56Z

Hey! Sorry I haven't had a moment to take a look yet, I needed to focus on some other project management tasks first to release 0.1.6 and clean up the PR queue. I'll give it a review sometime during the week

No worries, thanks for the update!

aaronleopold

The code itself looks good to me! I'll load this up locally to actually try it out before giving a final approval :)

…oToPage.tsx Dropping over-explanation. Co-authored-by: Aaron Leopold <36278431+aaronleopold@users.noreply.github.com>

LindaMukundwa · 2026-08-05T23:27:10Z

The code itself looks good to me! I'll load this up locally to actually try it out before giving a final approval :)

Thank you for the suggestions, and that is totally fair, just committed the changes!

aaronleopold · 2026-08-08T18:35:51Z

Time permitting, I'll aim to load this up locally today

aaronleopold

I had a couple of nit-picky comments, let me know what you think!

aaronleopold · 2026-08-08T19:47:45Z

+					autoFocus
+				/>
+				<Button size="sm" onClick={handleSubmit}>
+					<ChevronsRight className="mr-1 h-4 w-4" />


Can we remove the icon here? I think it just looks better without it, and for RTL reading it wouldn't necessarily be the ideal direction either:

Also, while looking at the screenshot, the button aligned to end looks a bit funky. We can't just set the Popover.Content to have items-center because the input has a label that throws off the alignment overall.

I'd say we should do the following:

Add an inner div to wrap the input and button with the proper flex alignment

Remove the label on the Input and add it, instead, to above the aforementioned group so it doesn't interfere with alignment

It's a pretty small thing though, so if you don't have the time to do it just let me know :)

Hi! No worries at all, thanks for the comments, and please tell me if you want to change anything else. I believe it should have changed, and correct me if I didn't properly fix the alignment issue you see on the Go button.

I also didn't consider RTL reading, so thank you for pointing that out. I tried to implement the changes and think they should be correct, but let me know if they are not, including what I should specifically fix. Thanks! :D

…l above input/button row

aaronleopold · 2026-08-10T21:29:12Z

@@ -23,9 +18,7 @@ export type GoToPageProps = {
 	 * localized as a whole.
 	 */
 	label?: string
-	/**
-	 * Text for the submit button. See `label` re: localization.
-	 */
+	//Text for the submit button. See `label` re: localization //


Looks like a few of these comments were accidentally updated, they are no longer valid doc comments

Just changed them!

aaronleopold

Thanks!

LindaMukundwa added 2 commits July 21, 2026 14:49

Added a feature for paged only where you can have a go-to-page contro…

da3a568

…l in the image-based reader so that you can get back to where you need easily

LindaMukundwa force-pushed the feat/reader-go-to-page branch from 2c0ac1b to da3a568 Compare July 21, 2026 18:56

Merge branch 'nightly' into feat/reader-go-to-page

1260f68

aaronleopold reviewed Aug 4, 2026

View reviewed changes

Comment thread packages/browser/src/components/readers/imageBased/container/GoToPage.tsx Outdated

Update packages/browser/src/components/readers/imageBased/container/G…

3e088b2

…oToPage.tsx Dropping over-explanation. Co-authored-by: Aaron Leopold <36278431+aaronleopold@users.noreply.github.com>

Merge branch 'nightly' into feat/reader-go-to-page

50ef78e

aaronleopold reviewed Aug 8, 2026

View reviewed changes

LindaMukundwa and others added 2 commits August 10, 2026 01:34

🌌 Addressed changes in review to remove the Go button icon, lift labe…

0280b99

…l above input/button row

Merge branch 'nightly' into feat/reader-go-to-page

5b94fa2

aaronleopold reviewed Aug 10, 2026

View reviewed changes

🗒️ Fixing comments/documentation

3aceb65

aaronleopold approved these changes Aug 11, 2026

View reviewed changes

aaronleopold merged commit 310bf39 into stumpapp:nightly Aug 11, 2026
7 checks passed

Uh oh!

Uh oh!

Conversation

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

Uh oh!

Description

Summary

1. Feature: "Go to page" control

2. Fix: PDFium binding reuse + thread-safety ( Already fixed, can be ignored )

Screenshots

Ready?

Stump Contributor License Agreement

Uh oh!

aaronleopold commented Jul 20, 2026

Uh oh!

LindaMukundwa commented Jul 21, 2026

Uh oh!

LindaMukundwa commented Aug 3, 2026

Uh oh!

aaronleopold commented Aug 3, 2026

Uh oh!

LindaMukundwa commented Aug 3, 2026

Uh oh!

aaronleopold left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

LindaMukundwa commented Aug 5, 2026

Uh oh!

aaronleopold commented Aug 8, 2026

Uh oh!

aaronleopold left a comment

Choose a reason for hiding this comment

Uh oh!

aaronleopold Aug 8, 2026

Choose a reason for hiding this comment

Uh oh!

aaronleopold Aug 8, 2026

Choose a reason for hiding this comment

Uh oh!

LindaMukundwa Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

aaronleopold Aug 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LindaMukundwa Aug 11, 2026

Choose a reason for hiding this comment

Uh oh!

aaronleopold left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

LindaMukundwa commented Jul 20, 2026 •

edited

Loading

aaronleopold Aug 10, 2026 •

edited

Loading