fix(viewer): keep shared fitz handle valid, cancel renders before saveIncr, debounce search - #138
Merged
Merged
Conversation
…eIncr, debounce search Root-cause fixes for four viewer bugs from the adversarial audit: M2 - shared fitz handle closed underneath the panel. The canvas and the panel hold the SAME fitz.Document (canvas.load(doc)). In the failed delete-comment path the canvas closed+reopened self._doc but only updated its own reference, leaving panel._fitz_doc pinned to the closed Document so _do_search / _print_pdf raised "document closed". The canvas now exposes a doc_replaced(object) signal, emits the fresh handle after reopening, and the panel repoints _fitz_doc via _on_doc_replaced. Defensive guards added: _do_search and _print_pdf abort gracefully on a None/closed Document (is_closed check + RuntimeError/ValueError catch). saveIncr race - background _PageJob workers fitz.open() the same file on disk that saveIncr() appends to on the main thread, so a worker mid-open read a half-written incremental update. The delete path now bumps _gen, clears _pending and joins in-flight workers (waitForDone) before saveIncr, then reschedules renders afterwards. Search debounce - _do_search scanned every page synchronously on the UI thread on each keystroke, freezing large PDFs. Keystrokes are now coalesced through a 250 ms single-shot QTimer. Password-cancel state - cancelling the password prompt for a new encrypted PDF (after the previous doc was already closed) left an empty splitter with a stale title and live nav buttons. A new _reset_to_placeholder restores a coherent placeholder state. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ebounce teardown MINOR 1: the pre-saveIncr join used QThreadPool.globalInstance().waitForDone() with no timeout on the UI thread, and the global pool is shared with the editor canvas, so a pathological/unrelated render could pin the UI indefinitely. Give the viewer its own dedicated QThreadPool for _PageJob workers and extract the pre-save preparation into a testable _prepare_for_save() that bumps _gen (epoch guard invalidates late results), clears _pending and joins with a bounded 5s wait, returning False on timeout (safe to proceed past). MINOR 2: a successful load() of a new document did not stop the search debounce nor clear _pending_search_query, so a search scheduled before the swap could fire against the new doc. Add a shared _reset_search_state() helper (reused by _reset_to_placeholder) and call it in the load() teardown. MINOR 3: if the reopen after a failed saveIncr raised, self._doc was left pointing at the already-closed Document (latent use-after-close) and doc_replaced was never emitted. Extract _reopen_document(): publish self._doc = None before closing the old handle, only swap in the fresh Document after fitz.open succeeds, and emit doc_replaced(new_doc) — or doc_replaced(None) on a double failure so the panel drops the shared ref. Tests: replace the two weak source-substring tests (race guard, doc_replaced) with behavioural tests of _prepare_for_save (gen bump, pending clear, bounded timeout, dedicated pool) and _reopen_document (live-handle swap + old handle closed; double-failure leaves _doc None, not closed). Add a behavioural test that load() cancels a pending debounced search. Widen the saveIncr window in test_viewer_safety after the reopen refactor. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Deploying pdfapps with
|
| Latest commit: |
ee52fbe
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5afa6f48.pdfapps.pages.dev |
| Branch Preview URL: | https://fix-viewer-shared-doc-search.pdfapps.pages.dev |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Root-cause fixes for four viewer bugs found by adversarial audit. Scope is limited to
app/viewer/canvas.py,app/viewer/panel.pyand tests.M2 (MAJOR) — panel
_fitz_docleft pointing at a closed Documentcanvas._docandpanel._fitz_docare the samefitz.Document(self._canvas.load(doc, ...)). In the failed delete-comment path the canvas didself._doc.close()+ reopen but only updated its own reference;panel._fitz_dockept pointing at the closed handle, so_do_search/_print_pdfraisedRuntimeError: document closed.Fix (chosen approach): canvas → panel signal. The canvas already owns the close/reopen, so notifying the shared owner is the least-invasive, most robust option (vs. inverting ownership so the panel reopens). The canvas exposes
doc_replaced = Signal(object)and emits the fresh handle right afterself._doc = new_docin thesaveIncrexcept block; the panel connects it and repoints_fitz_docin_on_doc_replaced. So no path leaves_fitz_docon a closed doc.Defense in depth:
_do_searchand_print_pdfnow abort gracefully if the handle isNone/closed (getattr(doc, "is_closed", False)guard +RuntimeError/ValueErrorcatch around the scan) instead of crashing.MINOR —
saveIncr↔ render worker raceBackground
_PageJobworkersfitz.open(self._path)from disk while the main thread doesself._doc.saveIncr()(append) to the same file — a worker opening mid-append reads a half-written incremental update (parse failure / Windows sharing violation). BeforesaveIncr, the delete path now bumps_gen, clears_pendingand joins in-flight workers viaQThreadPool.waitForDone(), then reschedules visible pages afterwards (both the success and the reopen paths). Follows the existing generation/epoch pattern.MINOR — search ran on the UI thread per keystroke
_on_search_text_changed→_do_searchscanned every page synchronously on each keystroke, freezing the UI for seconds on large PDFs. Added a 250 ms single-shotQTimerdebounce (same pattern as the thumbnail scroll timer); empty query still resets immediately and cancels the pending scan. Results are unchanged, only responsiveness.MINOR — inconsistent state on password cancel
Replacing an open document with a new encrypted one and cancelling the password prompt left the previous doc closed but the splitter still visible,
_name_lblstale and nav buttons active. New_reset_to_placeholder()restores a coherent placeholder state (placeholder shown, splitter/sidebar hidden, title + page label reset, nav disabled, recents rebuilt) and is called on the cancelreturn.Tests (
tests/test_viewer_shared_doc.py, 9 tests, offscreen)doc_replaced.emitrepointspanel._fitz_docand search then works;_do_search/_print_pdfsurvive a closed handle.QMenu.execspins a native popup loop that PySide won't let us monkeypatch, so the delete context-menu can't be driven headless — documented in the test.setTextcalls trigger_do_searchonce (after the timer), with correct results; empty query cancels the pending scan.Two byte-window guards in
tests/test_viewer_safety.pywere widened (the added code shifted offsets; the asserted strings are all still present).Validation
pytest -q: 483 passed, 3 skipped, 0 failed.ruff check --select F,E9on the three scope files + new test: clean.🤖 Generated with Claude Code