fix(viewer): suppress stale PDF render publication - #323
Conversation
📝 WalkthroughWalkthroughPDF 인라인 렌더링이 ChangesPDF 렌더링 취소 처리
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Polling
participant Viewer
participant PDFjs
participant PreviewDOM
Polling->>Viewer: PDF 렌더링 요청 및 AbortSignal 전달
Viewer->>PDFjs: PDF 로드, 페이지 조회, 캔버스 렌더링
Polling->>Viewer: 렌더링 후 취소 상태 확인
Viewer-->>Polling: false 반환
Polling-->>PreviewDOM: 링크와 완료 상태를 게시하지 않음
Possibly related issues
🚥 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 |
|
@opencode-agent @cwl-noema-review Please independently review exact current head |
|
@coderabbitai review |
|
@coderabbitai review Please review exact current head |
|
🧩 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
No findings.
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
|
d7a3342 to
4ea2cce
Compare
|
@opencode-agent @cwl-noema-review Please independently review exact current head |
Pull request was converted to draft
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/test_viewer_render_cancellation.py (1)
151-156: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win마이크로태스크 개수에 의존하면 검증 대상 경로가 달라집니다.
renderPdfInline은 abort 전에getPdfJsModule(),loadingTask.promise,pdfDocument.getPage(1)을 순차로 await합니다.getPage가async함수이므로 필요한 마이크로태스크 수가 두 번을 초과할 수 있습니다. 이 경우 abort는 Line 160의getPage이후 확인에서 감지됩니다. assert는 통과하지만 렌더 완료 후 게시 억제 경로(viewer.js Line 183-186)는 검증되지 않습니다.
render()호출 시점을 관측 가능한 신호로 만들고 그 신호를 await한 후 abort하십시오. 그러면 검증 대상 경로가 결정됩니다.♻️ 제안 수정
+ let renderStarted; + const renderStartedPromise = new Promise(resolve => { + renderStarted = resolve; + }); let resolveRender; const renderPromise = new Promise(resolve => { resolveRender = resolve; }); const pdfDocument = { numPages: 1, async getPage() { return { getViewport({ scale }) { return { width: 100 * scale, height: 200 * scale }; }, render() { + renderStarted(); return { promise: renderPromise }; }, }; }, async destroy() {}, };- // Let getDocument/getPage reach the deliberately unresolved render promise. - await Promise.resolve(); - await Promise.resolve(); + // Wait until page.render() actually starts, then supersede the operation. + await renderStartedPromise; controller.abort(); resolveRender(); - await rendering; + assert.equal(await rendering, false, "an aborted render must report failure");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/test_viewer_render_cancellation.py` around lines 151 - 156, Update the cancellation test around renderPdfInline so it observes a signal when render() is invoked, awaits that signal, and only then calls controller.abort() and resolveRender(). Remove the fixed double Promise.resolve() scheduling, ensuring the test deterministically exercises the post-render publication-suppression path rather than relying on getDocument/getPage microtask timing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/test_viewer_render_cancellation.py`:
- Around line 171-186: Convert the module-level
test_superseded_pdf_render_does_not_publish function into a method on a
unittest.TestCase subclass, preserving its existing Node.js harness execution
and assertions. Ensure the module imports unittest and the resulting test class
and method are discoverable by python3 -m unittest discover -s scripts.
---
Nitpick comments:
In `@scripts/test_viewer_render_cancellation.py`:
- Around line 151-156: Update the cancellation test around renderPdfInline so it
observes a signal when render() is invoked, awaits that signal, and only then
calls controller.abort() and resolveRender(). Remove the fixed double
Promise.resolve() scheduling, ensuring the test deterministically exercises the
post-render publication-suppression path rather than relying on
getDocument/getPage microtask timing.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c609e833-b01a-4e9c-a495-1ef3b4e6b369
📒 Files selected for processing (2)
scripts/test_viewer_render_cancellation.pysrc/main/resources/static/assets/viewer/viewer.js
| def test_superseded_pdf_render_does_not_publish() -> None: | ||
| """Abort an in-flight render and require zero stale DOM publication.""" | ||
|
|
||
| node = shutil.which("node") | ||
| assert node is not None, "Node.js is required for the viewer runtime regression" | ||
|
|
||
| result = subprocess.run( | ||
| [node, "-e", NODE_HARNESS, str(VIEWER_SOURCE)], | ||
| cwd=REPOSITORY_ROOT, | ||
| capture_output=True, | ||
| check=False, | ||
| text=True, | ||
| timeout=15, | ||
| ) | ||
|
|
||
| assert result.returncode == 0, result.stdout + result.stderr |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
unittest 스위트로 감싸야 합니다.
이 파일은 모듈 수준 pytest 스타일 함수만 정의합니다. unittest.TestCase 서브클래스가 없습니다. 따라서 python3 -m unittest discover -s scripts는 이 모듈을 임포트하지만 테스트를 0개 수집합니다. 회귀 검증이 표준 명령에서 조용히 누락됩니다.
unittest.TestCase 기반으로 변환하십시오.
코딩 가이드라인에 따릅니다: "Python helper scripts use the standard-library unittest suite and must pass python3 -m unittest discover -s scripts".
♻️ 제안 수정
+import unittest
import shutil
import subprocess
from pathlib import Path-def test_superseded_pdf_render_does_not_publish() -> None:
- """Abort an in-flight render and require zero stale DOM publication."""
-
- node = shutil.which("node")
- assert node is not None, "Node.js is required for the viewer runtime regression"
-
- result = subprocess.run(
- [node, "-e", NODE_HARNESS, str(VIEWER_SOURCE)],
- cwd=REPOSITORY_ROOT,
- capture_output=True,
- check=False,
- text=True,
- timeout=15,
- )
-
- assert result.returncode == 0, result.stdout + result.stderr
+class ViewerRenderCancellationTest(unittest.TestCase):
+ def test_superseded_pdf_render_does_not_publish(self) -> None:
+ """Abort an in-flight render and require zero stale DOM publication."""
+
+ node = shutil.which("node")
+ if node is None:
+ self.skipTest("Node.js is required for the viewer runtime regression")
+
+ result = subprocess.run(
+ [node, "-e", NODE_HARNESS, str(VIEWER_SOURCE)],
+ cwd=REPOSITORY_ROOT,
+ capture_output=True,
+ check=False,
+ text=True,
+ timeout=15,
+ )
+
+ self.assertEqual(result.returncode, 0, result.stdout + result.stderr)
+
+
+if __name__ == "__main__":
+ unittest.main()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_superseded_pdf_render_does_not_publish() -> None: | |
| """Abort an in-flight render and require zero stale DOM publication.""" | |
| node = shutil.which("node") | |
| assert node is not None, "Node.js is required for the viewer runtime regression" | |
| result = subprocess.run( | |
| [node, "-e", NODE_HARNESS, str(VIEWER_SOURCE)], | |
| cwd=REPOSITORY_ROOT, | |
| capture_output=True, | |
| check=False, | |
| text=True, | |
| timeout=15, | |
| ) | |
| assert result.returncode == 0, result.stdout + result.stderr | |
| import unittest | |
| class ViewerRenderCancellationTest(unittest.TestCase): | |
| def test_superseded_pdf_render_does_not_publish(self) -> None: | |
| """Abort an in-flight render and require zero stale DOM publication.""" | |
| node = shutil.which("node") | |
| if node is None: | |
| self.skipTest("Node.js is required for the viewer runtime regression") | |
| result = subprocess.run( | |
| [node, "-e", NODE_HARNESS, str(VIEWER_SOURCE)], | |
| cwd=REPOSITORY_ROOT, | |
| capture_output=True, | |
| check=False, | |
| text=True, | |
| timeout=15, | |
| ) | |
| self.assertEqual(result.returncode, 0, result.stdout + result.stderr) | |
| if __name__ == "__main__": | |
| unittest.main() |
🧰 Tools
🪛 ast-grep (0.45.1)
[error] 176-183: Command coming from incoming request
Context: subprocess.run(
[node, "-e", NODE_HARNESS, str(VIEWER_SOURCE)],
cwd=REPOSITORY_ROOT,
capture_output=True,
check=False,
text=True,
timeout=15,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🪛 Ruff (0.16.1)
[error] 177-177: subprocess call: check for execution of untrusted input
(S603)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/test_viewer_render_cancellation.py` around lines 171 - 186, Convert
the module-level test_superseded_pdf_render_does_not_publish function into a
method on a unittest.TestCase subclass, preserving its existing Node.js harness
execution and assertions. Ensure the module imports unittest and the resulting
test class and method are discoverable by python3 -m unittest discover -s
scripts.
Source: Coding guidelines
1 participant
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
4ea2cce1a07462185ada525513de39c90cb9e085;main:55d7ae8647208e301f282350f076eeddaba61d11after protected merge of fix(security): harden audit pseudonymization and refresh Netty evidence #270;viewer.jswas byte-identical to the predecessor fix(viewer): suppress stale PDF render publication #323 base before applying the delta, proving fix(security): harden audit pseudonymization and refresh Netty evidence #270 introduced no path-local conflict;79689f928d8552c65dbdecf85ed45e2e9a6f1109;31390258726: success;31390258088: success;31390257890: success;31390257815: success;Test-first evidence
RED
Historical test-only head
45b1f9add3424228bf690b8009a92dff553c74e7addedscripts/test_viewer_render_cancellation.pywhile productionrenderPdfInlinestill ignored the operation abort signal. The behavioral Node harness held an oldpage.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.
Summary by CodeRabbit
버그 수정
테스트