Repository navigation
Conversation
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Track failures and schedule eligible work crates/ui-egui/src/canvas.rs, crates/ui-egui/src/canvas/cache.rs, crates/ui-egui/src/canvas/organize.rs, crates/ui-egui/src/canvas/tests.rs, ROADMAP.md, docs/roadmap.md |
Failures are recorded and suppressed per request and domain. History is bounded by current demand. Successful results can clear matching failures. Eligible main, thumbnail, grid, and print work can proceed independently. Tests and roadmap notes cover these changes. |
Render cached content with page errors crates/ui-egui/src/canvas.rs, crates/ui-egui/src/canvas/tests.rs |
The canvas checks for visible cached pixels before displaying a full-page error. It paints retained tiles and shows an error indication when failed content remains visible. Tests cover tile-result order, cached backdrops, and pages without visible pixels. |
Estimated code review effort: 4 (Complex) | ~45 minutes
Change: Bug fix
Suggested reviewers: fredapps, aiongg
Merge Risk: 
1abf5
-
The change keeps successfully rendered page pixels visible with an error notice when another tile or backdrop fails. No merge-blocking issue remains from the review; the author's deterministic regression tests cover the scheduling and cache logic.
Security architecture risk: 
c5d15
-
The change preserves partial page content while separating failures between the main view and previews. Existing rendering safeguards remain in the examined paths, and no introduced security defect was established. The assessment is limited to those paths rather than a complete security assurance.
Retained concerns
No architecture-level concerns identified.Security review details
-
Security Blast Radius
- inferred — For the examined paths, document-induced render failures affect the receiving DocView's page caches, scheduling and diagnostics across its rendering consumers. Continued work expands beyond the formerly failed request, but no additional file, network, credential or cross-document authority was established.
Trust Boundaries and Controls
- observed — Continued scheduling after failure does not bypass the existing result-admission boundary. Request-value membership in the latest queue rejects obsolete results, and successful pixel buffers still undergo size and admission checks before texture creation. These are retained controls, not a new request-identity mechanism.
Resilience and Maintainability Implications
- inferred — Request-specific suppression improves failure containment by avoiding repeated identical failures without stopping independent rendering consumers. Inspected regression cases support this behavior under partial failure, saturation, invalidation and stale-result delivery; they do not establish exhaustive resource-exhaustion resistance.
-
Pre-merge checks: 
-
Check name Status Explanation Title check The title clearly identifies the UI fix: preserving rendered page tiles after partial render failures. It is specific and concise enough for project history. Description check The description covers the requested sections, explains the behavior and scope, documents verification results and limitations, and addresses never-crash, provenance, security, parity, and reviewer fo… Docstring Coverage Docstring coverage is 96.88% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentati… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Description check
-
Explanation
The description covers the requested sections, explains the behavior and scope, documents verification results and limitations, and addresses never-crash, provenance, security, parity, and reviewer focus.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 96.88% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 32 functions across 4 files. (2 skipped: 2 unsupported.)
-
Finishing Touches
-
Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
RevisãoBoa melhoria para o #623 item 4 (só viewer, como o PR delimita). Revisei o que dá por leitura:
Não consigo verificar a corretude do agendamento/cache inteira por leitura — mas a cobertura é extensa (ordenação, zoom/pan, retenção limitada, agendamento de preview, cobertura offscreen, recuperação, invalidação, resultados stale), e é esse conjunto de regressões determinísticas que serve de backstop para lógica de UI assim. Relação: este PR é de cache de tiles de página, independente dos #5237/#5233/#5318 (pintura de padrão de ladrilho) — camadas diferentes, não concorrem. Observação: merge |
FredApps
left a comment
FredApps
left a comment
There was a problem hiding this comment.
ReviewerA — source-read COMMENT (not an approval)
I read the diff: rendered page tiles are preserved after partial render failures (#623 item 4). The viewer's errors map changes from one message per page that was "never re-requested" to a bounded retry history per page (HashMap<usize, Vec<(RenderRequest, String)>>), only the current demand contributes diagnostics (error_demand, the last committed pixel demand including cached and failed requests before queue filtering), and a failed request leaves the successfully rendered visible tiles and backdrops on screen with an error notice, collecting demand after a failure because another in-flight tile can still succeed; a failed zoom-out may show the older tiles below the tiling threshold. Not blanking a page because one tile failed is the right behaviour for a viewer. The ROADMAP carries three log lines of this PR's own follow-ups. The risk is retry storms: a request that fails must not be re-sent every frame (the old comment said "never re-requested"; the new one says "bounded retry history"), so state the bound (a count and a backoff), and test that a page whose tile always fails stops asking after N tries. I read about 28 lines. I have not run it, and no exact-head CI result exists for this head, so this is a source-read comment and not an approval. Nothing was run.
I ran nothing and have no exact-head CI result for this PR, so this is not an approval. Where a diff was long I read its code hunks rather than every generated or documentation line.
Status: waiting_author.
|
Thanks for the review, @FredApps. The retry-storm concern is addressed through exact-request failure suppression, rather than an N-attempt retry loop with timed backoff.
The regressions The updated PR is now mergeable against upstream main. Local Windows validation passed 182 distinct tests, including the combined image-drag/partial-render-failure regression, with strict Clippy and formatting checks passing. Upstream CI still requires maintainer approval, so I am not claiming exact-head CI success. |
What this changes
Refs #623, item 4 (viewer behavior only). This partial fix leaves the issue's other items open.
A failed pixel request currently replaces a whole page with an error even if another tile or backdrop rendered successfully. This change preserves useful visible pixels and shows an error notice beside them. When no cached pixels intersect the visible page region, the existing full-page error remains.
page_errors(), so a failed preview cannot put a false warning on a successfully rendered main page.max(64, current pixel demand), then trim it as demand changes or ends. An untried healthy tile can progress even after 64 other visible requests fail.No watchdog budgets, PDF interpretation, raster-cache architecture, GPU rendering or localization changes. No speedup is claimed.
How you verified it
Windows 11 x86-64, Rust/Cargo 1.98.1. Cargo checks used the low-disk profile, locked/offline dependencies and two jobs. Visual tests used Vulkan and
PDFCRAFT_SYSTEM_FONTS=0.The build used the project's existing CI font input: craft-fonts at b076417, with
CRAFT_FONTS_REQUIRED=1. Its manifest licences and SHA-256 hashes were verified. No font files or font-policy changes enter this PR.cargo fmt --checkandgit diff --check.cargo clippy -p pdfcraft-ui-egui --all-targets -- -D warnings.tile_regressions: 17 passed.--test it: 1,190 passed, 0 failed, 7 ignored.--test it: 43 passed, 1 ignored.cargo xtask layers,cargo xtask assets,cargo xtask parity.cargo check -p pdfcraft-ui-egui --target wasm32-unknown-unknown(five existing renderer warnings and nine existing UI warnings in untouched code).Archived red/green runs demonstrate the original pixel-loss bug and the independently reviewed follow-up failures with their corresponding corrections absent. The final focused selection also protects existing invalidation and rendering behavior. Its coverage includes both tile arrival orders, a successful backdrop, all failures, preview independence, main/preview diagnostics, zoom/pan, visible coverage at all four rotations, invalidation, stale results and recovery. The synthetic stress test exercises 512 distinct requests. Actual headless canvas geometry produces 81 visible tiles plus a backdrop and verifies continued scheduling after 64 failures; no giant framebuffer is allocated.
Font-baseline distinction: earlier runs without craft-fonts recorded full UI library 567 passed/17 failed/1 ignored, viewer 65/1, view 75/4 and control 58/1, with the known U+22EF failure sets. Those archived runs remain failures. With the documented CI input, unchanged main at
7555be3cfirst passed 570 UI library tests (0 failed, 1 ignored). Main advanced during validation, first to21f6c29dand thendc943eeb. The scoped commit integrated cleanly; range-diff and exact added/deleted-line comparison confirm the reviewed patch is unchanged. The final counts above are fromdc943eeb. No assertions or missing-glyph policy were relaxed.Not run: full-workspace sweep, other-OS runtime, live browser runtime, fuzzing, real heavy-PDF end-to-end reproduction, a physical oversized-window/downlevel-adapter experiment, or comparable before/after benchmarks. The default graphics backend was not rerun; no resolution of its previously observed crash is claimed. The saturation case establishes headless layout/scheduling behavior, not native GPU throughput.
Never-crash
unsafe.Provenance
Security
Parity and reach
page_errors()diagnostics.ROADMAP.mdanddocs/roadmap.mdupdated. No new parity capability is claimed and no parity entry is changed.Anything a reviewer should look at first
Review the request-history lifecycle and main-view diagnostic selection in
canvas/cache.rs, and visible pixel coverage/error painting incanvas.rs.PR #4501 remains open and handles successful renders with content warnings. It shares
canvas.rs,canvas/cache.rsandcanvas/tests.rs; expect source conflicts if both patches land. Preserve both itswarnings/warnings_dismissedbehavior and this PR'serrors/error_demandhandling when reconciling them. #4191 (watchdog deadlines) and #2113 (truncation notices) remain separate work. No other contributor's branch was changed.Summary by CodeRabbit