Skip to content

fix(viewer): suppress stale PDF render publication - #323

Open
seonghobae wants to merge 6 commits into
mainfrom
fix/viewer-generation-safe-rendering
Open

fix(viewer): suppress stale PDF render publication#323
seonghobae wants to merge 6 commits into
mainfrom
fix/viewer-generation-safe-rendering

Conversation

@seonghobae

@seonghobae seonghobae commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator

Objective

Advance issue #322 with a bounded viewer-runtime race fix. A superseded PDF.js render may finish internally, but it must not append stale canvas/metadata or publish later preview/link/Ready. state after a newer viewer operation has started.

Fresh exact state — 2026-08-10

Test-first evidence

RED

Historical test-only head 45b1f9add3424228bf690b8009a92dff553c74e7 added scripts/test_viewer_render_cancellation.py while production renderPdfInline still ignored the operation abort signal. The behavioral Node harness held an old page.render().promise, aborted its operation, then resolved the old render. The required contract was zero stale canvas/metadata publication.

GREEN and clean-base proof

renderPdfInline(path, abortSignal) now checks cancellation around PDF.js loading, page acquisition, render completion, and every subsequent DOM publication boundary. poll(...) passes the operation signal and stops later preview link and terminal ready-state publication after supersession.

Clean exact output identity:

  • src/main/resources/static/assets/viewer/viewer.js: 33ec05397abf8cb90aba312219e5e7a1af7da6a5;
  • scripts/test_viewer_render_cancellation.py: 307f8a8f21c5e6e25dc32e938d8ec650dcd26773.

Scope / remaining issue contract

This slice is path-disjoint from #264 demo.js/dom-utils.js, #318 Java controller branding, #306 Office conversion, #313 HMAC readiness, and #316/#337 OpenAPI contracts. Issue #317 separately owns viewer/session authorization semantics.

Do not close issue #322 when this PR integrates. Remaining acceptance includes active RenderTask.cancel() and loading-task destruction where supported, rapid multi-generation/error-state coverage, signed-token-mode lifecycle parity, terminal status/focus ownership, and no unhandled cancellation rejection.

Merge gate

Keep this exact head unchanged. Auto-merge may complete only after live repository protection still sees all required checks passing, zero valid unresolved findings, and the counted approving review from a qualifying independent reviewer with write access. Automated evidence is not approval.

@coderabbitai

coderabbitai Bot commented Aug 10, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@seonghobae, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 27 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 41a4aadd-0ae1-46c5-ba13-822bc8ceef80

📥 Commits

Reviewing files that changed from the base of the PR and between 55d7ae8 and 8914580.

📒 Files selected for processing (2)
  • scripts/test_viewer_render_cancellation.py
  • src/main/resources/static/assets/viewer/viewer.js

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Copy link
Copy Markdown
Collaborator Author

@opencode-agent @cwl-noema-review Please independently review exact current head 79689f928d8552c65dbdecf85ed45e2e9a6f1109 against protected main 83ec6f7fe2b04bdcd28bf98ec350e41e55730a18. Exact-head CI 31374483914, Security Scan 31374483892, SAST Semgrep 31374483824, and fuzz 31374483908 are successful; no inline review thread is currently returned. Verify the bounded stale-render publication fix: superseded PDF.js work may finish internally but must not append stale canvas/metadata or publish later preview/Ready state after the operation abort signal. Review the behavioral Node regression and shipped viewer.js; keep issue #317 demo-auth authority separate. Submit a formal review only for this exact head if supported; do not mutate the branch, merge, or weaken gates.

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

Please review exact current head 79689f928d8552c65dbdecf85ed45e2e9a6f1109. Focus on whether abort-generation checks fully prevent stale PDF.js canvas/metadata/Ready publication without weakening current viewer behavior. Treat predecessor-head evidence as historical.

@coderabbitai

coderabbitai Bot commented Aug 10, 2026

Copy link
Copy Markdown
⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai

coderabbitai Bot commented Aug 10, 2026

Copy link
Copy Markdown
🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

head_sha="$(git rev-parse HEAD)"
base_sha="83ec6f7fe2b04bdcd28bf98ec350e41e55730a18"
expected_head="79689f928d8552c65dbdecf85ed45e2e9a6f1109"

printf 'HEAD=%s\nEXPECTED_HEAD=%s\nBASE=%s\n' "$head_sha" "$expected_head" "$base_sha"
git diff --check "${base_sha}..${expected_head}"
git diff --stat "${base_sha}..${expected_head}"
git diff --name-status "${base_sha}..${expected_head}"

printf '\n== Locate candidate files ==\n'
fd -HI '^(viewer\.js|test_viewer_render_cancellation\.py)$' . \
  -E .git -E node_modules -E dist -E build || true

printf '\n== JavaScript structure ==\n'
viewer_file="$(fd -HI '^viewer\.js$' . -E .git -E node_modules -E dist -E build | head -n 1)"
if [ -n "$viewer_file" ]; then
  ast-grep outline "$viewer_file" --items all --type function --match 'renderPdfInline|poll' --view expanded || true
  printf '\n== Relevant source occurrences ==\n'
  rg -n -C 16 -P '\b(renderPdfInline|poll)\s*\(|\.render\s*\(|appendChild\s*\(|textContent\s*=|innerHTML\s*=|ready|preview|abortSignal|aborted)\b' "$viewer_file"
fi

printf '\n== Exact source diff ==\n'
git diff --find-renames --find-copies --unified=80 "${base_sha}..${expected_head}" -- \
  ':(glob)**/viewer.js' \
  ':(glob)**/test_viewer_render_cancellation.py'

Length of output: 2440


🏁 Script executed:

#!/bin/bash
set -euo pipefail

base_sha="83ec6f7fe2b04bdcd28bf98ec350e41e55730a18"
head_sha="79689f928d8552c65dbdecf85ed45e2e9a6f1109"
viewer="src/main/resources/static/assets/viewer/viewer.js"
test_file="scripts/test_viewer_render_cancellation.py"

printf '%s\n' '== viewer.js: imports, state, and renderPdfInline =='
sed -n '1,235p' "$viewer" | cat -n

printf '%s\n' '== viewer.js: poll and subsequent state publication =='
sed -n '236,430p' "$viewer" | cat -n

printf '%s\n' '== Regression test =='
cat -n "$test_file"

printf '%s\n' '== Exact diff =='
git diff --check "${base_sha}..${head_sha}"
git diff --unified=60 "${base_sha}..${head_sha}" -- "$viewer" "$test_file"

printf '%s\n' '== All DOM/state publication calls in viewer.js =='
rg -n -C 5 -e 'appendChild\s*\(' -e 'replaceChildren\s*\(' -e '\.textContent\s*=' \
  -e '\.innerHTML\s*=' -e 'setAttribute\s*\(' -e 'updateStatus\s*\(' \
  -e 'renderPdfInline\s*\(' -e 'await\s+' "$viewer"

Length of output: 42342


@seonghobae Reviewed exact head 79689f928d8552c65dbdecf85ed45e2e9a6f1109 against protected base 83ec6f7fe2b04bdcd28bf98ec350e41e55730a18.

No findings.

renderPdfInline(path, abortSignal) checks cancellation after each asynchronous PDF.js boundary. The check after page.render(...).promise occurs before canvas and metadata publication.

poll(docId, abortSignal) passes the operation signal to the renderer. It stops before preview-link publication after the render. It also stops before busy-state and Ready. publication.

The Node regression holds the PDF.js render promise, aborts the operation, completes the old render, and verifies zero preview children. This test covers stale canvas and metadata publication.

The change does not modify the separate signed external-artifact path or #317 demo-auth behavior.

⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@seonghobae
seonghobae force-pushed the fix/viewer-generation-safe-rendering branch from d7a3342 to 4ea2cce Compare August 10, 2026 12:54

Copy link
Copy Markdown
Collaborator Author

@opencode-agent @cwl-noema-review Please independently review exact current head 4ea2cce1a07462185ada525513de39c90cb9e085 against protected main 55d7ae8647208e301f282350f076eeddaba61d11. Exact-head CI 31390258726, Security Scan 31390258088, SAST 31390257890, and fuzz 31390257815 are successful. Verify superseded PDF.js work cannot append stale canvas/metadata or later preview/Ready state after abort, the behavioral Node harness exercises late render completion, and this remains only the first issue-#322 slice rather than active task cancellation/destruction or session-authority completion. Advisory review only; do not mutate, count as human approval, merge, or weaken gates.

@seonghobae
seonghobae marked this pull request as draft August 12, 2026 02:22
auto-merge was automatically disabled August 12, 2026 02:22

Pull request was converted to draft

@seonghobae
seonghobae marked this pull request as ready for review August 12, 2026 03:44
@seonghobae
seonghobae enabled auto-merge (squash) August 12, 2026 03:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant