Repository navigation
Conversation
pdftoppm wrote PNG, so every page turn paid for libpng compression in poppler and for the PNG decode on the way back in. ddjvu already wrote PPM, which ppmToPNG re-encoded as PNG for the same decode. Register a binary PPM decoder with the image package and let both converters write PPM. The page pipeline decodes it in one pass. PageBytes converts PPM to PNG for the browser, the one consumer that needs an encoded image. ppmToPNG goes away.
pdftotext and pdftohtml each parse the document for one page. The link pass waited for the word pass to finish; start it first and join the two when the words are in.
pdftohtml writes page destinations as name.html#N. Cutting at the first '#' lost every internal link in a PDF whose file name contains one. Cut at the last.
The prefetch after a frame warmed one page past the current spread, so each spread turn rendered its second page cold. For PDF and DJVU that is a converter run on the critical path.
Picking a bookmark in another book opened that book and jumped to the page. When the open failed, the jump still ran against the book already in the pane.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 49 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (6)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe changes update PPM page rendering and PNG responses, PDF text and link processing, and UI page prefetching and menu navigation. ChangesPPM page flow
PDF text and links
Page prefetching
Menu book navigation
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant pdfTextLayer
participant pdftotext
participant pdftohtml
participant pdfLinks
pdfTextLayer->>pdftotext: Run text extraction
pdfTextLayer->>pdftohtml: Start XML extraction concurrently
pdftotext-->>pdfTextLayer: Return text output
pdftohtml-->>pdfTextLayer: Return XML output through buffered channel
pdfTextLayer->>pdfLinks: Pass XML output
Merge Risk: ⚪ Minimal · up to No actionable merge-blocking issue remains. The rendering and navigation changes are mergeable after normal checks; canceling unused PDF link extraction is optional cleanup. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to Page-size limits and link filtering remain intact, and failed bookmark opens now preserve the current book and page. However, browser page requests now repeat image conversion independently, allowing concurrent requests to multiply memory and CPU work. The demonstrated exposure is limited to the local reader process. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟡 Medium book/pdftext.go:15
When pdftotext fails, pdfTextLayer returns nil while pdftohtml keeps running for up to toolTimeout, consuming a converter process whose result is no longer used. Cancel a child context and wait for the pdftohtml goroutine before returning from this error path.
n := strconv.Itoa(page)
+ toolCtx, cancel := context.WithCancel(ctx)
+ defer cancel()
// pdftohtml parses the document again; let it run alongside pdftotext.
linkXML := make(chan []byte, 1)
go func() {
- out, _ := runToolContext(ctx, "pdftohtml", "-xml", "-i", "-q", "-zoom", "1", "-f", n, "-l", n, "-stdout", path)
+ out, _ := runToolContext(toolCtx, "pdftohtml", "-xml", "-i", "-q", "-zoom", "1", "-f", n, "-l", n, "-stdout", path)
linkXML <- out
}()
- out, err := runToolContext(ctx, "pdftotext", "-cropbox", "-bbox-layout", "-f", n, "-l", n, path, "-")
+ out, err := runToolContext(toolCtx, "pdftotext", "-cropbox", "-bbox-layout", "-f", n, "-l", n, path, "-")
if err != nil {
+ cancel()
+ <-linkXML
return nil
}🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @internal/book/pdftext.go around lines 15-25:
When `pdftotext` fails, `pdfTextLayer` returns `nil` while `pdftohtml` keeps running for up to `toolTimeout`, consuming a converter process whose result is no longer used. Cancel a child context and wait for the `pdftohtml` goroutine before returning from this error path.
Evidence trail:
`internal/book/pdftext.go:16-36` (d00be63); `internal/book/tool.go:16-21,38-75` (d00be63); `internal/book/select.go:108-159` (d00be63); `internal/book/book.go:216-236` (d00be63).
There was a problem hiding this comment.
Fixed in 673b29d. pdfTextLayer now derives a child context and cancels it on return, so an early exit kills pdftohtml instead of letting it run to completion or timeout.
There was a problem hiding this comment.
The child context cancels pdftohtml, but early returns still do not wait for its goroutine to exit. Please receive from linkXML after cancellation before returning, as proposed, so the converter cleanup completes before pdfTextLayer returns.
There was a problem hiding this comment.
Leaving the early returns as they are. The goroutine's only effect is a send into a buffered channel; cancellation kills pdftohtml and runToolLimited's Wait reaps it a few milliseconds later. Nothing reads the result or watches for the goroutine to finish, so blocking the caller on that reap would only delay PrepareText storing the empty layer. On the normal path the receive before pdfLinks already joins the two.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Render PDF and DJVU pages as PPM instead of PNG. pdftoppm paid for libpng compression on every page and the reader paid for the PNG decode on the way back in; ddjvu already wrote PPM, which the pipeline re-encoded as PNG for the same decode. A binary PPM decoder registered with the image package reads the converter output in one pass. PageBytes converts PPM to PNG for the browser, the one consumer that needs an encoded image. On a near-blank 2000px page, pdftoppm alone drops from 100ms to 32ms per call; pages with content compress slower, and the Go-side PNG decode goes away.
Summary by CodeRabbit
#.Note
Speed up PDF and DjVu page turns by caching PPM and prefetching next pages
pdftoppmandddjvuinstead of converting to PNG in-process; a new registered P6 PPM decoder in ppm.go handles decoding, andBook.PageBytesconverts cached PPM pages to PNG when served.pdftohtmllink extraction concurrently withpdftotext, cancelling it when text extraction fails. Relative links whose document name contains#and ends in a numeric fragment are now treated as internal page links.toolPagepage filenames now use the.ppmextension instead of.png.Macroscope summarized 673b29d.
The PR appears safe to merge; no outstanding findings remain.
Summary
The PR caches PDF and DJVU pages as PPM, converts them to PNG when serving browser requests, starts PDF link extraction alongside text extraction, and adjusts page prefetching and navigation.
pdftohtmlprocess when text-layer preparation returns early.Diagram
%%{init: {'theme': 'neutral'}}%% flowchart LR A[PDF or DJVU converter] --> B[PPM page cache] B --> C[Image decoder] B --> D[PageBytes PNG conversion] D --> E[Browser]Reviews (2) · Last reviewed commit: "Stop the PDF link pass when the word pas..."