Skip to content

fix: 문서 교체 시 이전 PDFDocumentProxy를 destroy - #38

Merged
onetwothr1 merged 4 commits into
devfrom
fix/35-destroy-previous-pdf-document
Jul 28, 2026
Merged

fix: 문서 교체 시 이전 PDFDocumentProxy를 destroy#38
onetwothr1 merged 4 commits into
devfrom
fix/35-destroy-previous-pdf-document

Conversation

@onetwothr1

@onetwothr1 onetwothr1 commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

Summary

PdfHost.#setDocument()this.#doc = doc으로 덮어쓰기만 해서, PDFDocumentProxy.destroy()가 이 repo 어디에서도 불리지 않았다(grep -rn destroy src/ → 0건). pdf.js는 destroy() 전까지 워커 측 자원과 페이지 프록시를 붙들고 있어 GC 대상이 되지 않는다 — 한 세션에서 문서를 N번 열면 N개가 그대로 상주한다.

벤더링본 v2.14.0은 크롭을 살아 있는 캔버스로 유지하고 FigRenderError 감지도 없으므로, 이 누수는 Chrome이 캔버스 백킹 스토어를 회수하는 조건(엔진 백로그 B7)에 직접 기여한다. 그 증상은 오류도 재시도 버튼도 없이 프리뷰 카드가 백지로 뜨는 것이다. #34가 스캔 중첩을 줄였고, 이건 단조 증가하는 쪽이라 영향이 더 크다.

Closes #35

Changes

  • #setDocument — 이전 문서를 잡아 두고, linkService.setDocument(new)·viewer.setDocument(new) 이후에 destroy(). pdf.js의 PDFViewer.setDocument는 나가는 문서를 동기로 정리(render/textLayer cancel → _resetView)하므로 순서가 반대면 방금 파괴된 proxy를 만진다.
  • #releaseDocument — 정리 실패는 console.warn으로 삼켜 새 문서 로드를 막지 않는다.
  • #awaitLoad (신규) — 로드 실패 시 loadingTask.destroy(). pdf.js는 문서마다 전용 PDFWorker를 띄우는데 실패 시 promise만 reject하고 task·worker를 회수하지 않는다. 이 확장은 파일 없음·권한 거부 UX가 일급이라 실패 N번이 워커 스레드 N개로 쌓였다. 같은 누수 계열이라 함께 처리했다.
  • getOutlineItemsgetOutline() 거절을 삼키고 문서 교체를 감지해 빈 목록을 반환. 이게 없으면 이 PR이 회귀를 만든다: 원격 PDF의 outline이 아직 안 받은 range를 기다리는 중에 다른 PDF를 열면 destroy()가 그 조회를 거절시키고, 그 거절이 **이전 로드의 catch**로 흘러가 방금 정상 로드된 문서 위에 오류 화면을 띄운다(+ 그림·표 탭이 #doc = null로 죽는다).
  • #resolveDestPagedoc을 명시로 받는다. 교체 후 옛 dest를 새 문서에 풀지 않는다.
  • downloadCurrentPdf·jumpToOutlinedestroy()가 새로 노출시키는 거절을 삼킨다(이전에는 옛 문서로 resolve했다).
  • workerPort 금지 주석 — 설정하면 pdf.js가 PDFWorker.fromPort공유 싱글턴 워커를 넘기고, 그러면 한 문서 destroy가 다른 문서들의 워커까지 종료시킨다. 문서당 워커 1개가 이 코드의 전제다.

Test plan

  • npm run typecheck 통과 · vitest run 35/35 (7 파일) · npm run build 통과
  • test/pdf-host.test.ts 신규 — 이 repo의 첫 vi.mock 사용이다. 기존 6개 테스트는 pdf.js를 피한 순수 로직이었지만 이 결함은 pdf.js 생명주기 자체라 대역이 필요했다. 검증: 첫 로드 미파괴 / 두 번째 로드에서 이전 것만 1회·현재 것은 미파괴 / 전환 이후 순서 고정 / destroy 거절이 로드를 깨지 않음 / 로드 실패 시 loadingTask 정리 / 같은 인스턴스 재설정 시 미파괴.
  • 동어반복 아님을 확인: src/viewer/pdf-host.ts만 되돌리면 6개 중 4개가 실패한다(나머지 2개는 회귀 가드라 통과가 맞다).
  • 수동 QA는 안 했다 — 아래 절차를 리뷰어에게 요청한다.

수동 확인

같은 뷰어 창에 PDF를 드래그&드롭하는 것이 문서 교체의 사실상 유일한 경로다(파일 선택 대화상자는 오류 화면 버튼에만 걸려 있고, ?file= 파라미터는 페이지 로드 시 1회뿐이다).

  • 1. 정상 교체 — 논문 A를 열고, 그림·표 탭이 카드를 그린 뒤 논문 B를 드롭. B가 정상 렌더되고 콘솔에 경고·오류가 없어야 한다.
  • 2. 스캔 중 교체 — A의 그림·표 스캔이 끝나기 전에 B를 드롭. "figure 스캔에 실패했어요"가 뜨지 않고, B의 카드가 정상 렌더돼야 한다.
  • 3. 회귀 확인 (이 PR의 핵심 위험) — A를 열어 목차가 있는 것을 확인하고, A의 목차가 로드되는 도중 B를 드롭. 오류 화면이 뜨지 않아야 하고 B의 그림·표 탭이 살아 있어야 한다.
  • 4. 누수 확인 — DevTools → Memory에서 스냅샷을 찍고, 문서를 5~6번 교체한 뒤 다시 찍는다. 이전에는 PDFDocumentProxy/워커가 교체 횟수만큼 쌓였고, 이제는 그렇지 않아야 한다. Task Manager(Shift+Esc)에서 워커 프로세스 수를 봐도 된다.
  • 5. 실패 로드 — 존재하지 않는 로컬 파일을 열어 오류 화면을 띄운 뒤 정상 PDF를 연다. 정상 동작해야 하고, 실패를 여러 번 반복해도 워커가 쌓이지 않아야 한다.

남긴 것

  • stale-load 문제 → #37 신규. main.ts에 로드 epoch가 없어 겹친 로드에서 늦게 끝난 쪽이 새 문서 상태를 덮는다. 이 PR은 가장 날카로운 트리거(getOutlineItems)만 함수 내부에서 막았다 — 근본 해결은 #scanGeneration과 같은 모양의 epoch이고, 선재 결함이라 분리했다.
  • 엔진 쪽 미해결: 1차 패스의 await page.getTextContent() 안에서 문서가 destroy되면 그 promise가 영원히 settle되지 않는다(pdf.js worker가 task.terminated면 sink를 error 처리하지 않고, PDFPageProxy._destroy()는 텍스트 스트림을 추적하지 않는다). 그 스캔 frame이 pinned돼 페이지 텍스트 메타데이터가 상주한다 — 크롭 캔버스는 아니고, 근본 해결은 엔진이 getTextContent를 abort signal과 race시키는 것이라 Margin에서 막을 수 없다. 문서에 적어 뒀다.

Checklist

  • PR 제목이 Conventional Commits 형식 (feat: …, fix: … — squash 커밋 메시지가 됨)
  • npm run typecheck · npm test 통과
  • AI-assisted (에이전트가 작성/보조한 PR이면 체크)

🤖 Generated with Claude Code

Summary by CodeRabbit

Summary by CodeRabbit

  • 버그 수정
    • PDF 문서 교체 중 다운로드·목차 이동에서 발생하던 오류와 예외를 안정적으로 처리했습니다.
    • 중복/대체 로드가 일어날 때 오래된 로드 결과가 적용되거나 잔여 메타데이터가 남는 문제를 방지했습니다.
    • 로드 실패 시 로딩 작업 정리 및 화면 전환 중단 없이 동작하도록 개선했습니다.
  • 문서
    • 문서 결함 원인/해결 및 워커 사용 시 주의사항을 보강했습니다.
  • 테스트
    • 문서 교체·목차 동작 흐름을 검증하는 테스트를 추가했습니다.

PdfHost.#setDocument가 #doc을 덮어쓸 뿐이라 pdf.js가 워커 자원·페이지 프록시를
계속 붙들었다 — 한 세션에서 문서를 N번 열면 N개가 상주한다. 벤더링본 v2.14.0은
크롭을 살아 있는 캔버스로 유지하므로, Chrome이 메모리 압력에서 캔버스 백킹
스토어를 회수하는 조건(엔진 백로그 B7)에 직접 기여했다.

- #setDocument: 뷰어·linkService를 새 문서로 전환한 **뒤** 이전 문서 destroy.
  순서가 반대면 뷰어의 동기 teardown이 방금 파괴된 proxy를 만진다
- #releaseDocument: 정리 실패는 console.warn으로 삼켜 새 로드를 막지 않는다
- #awaitLoad: 로드 실패 시 loadingTask.destroy(). pdf.js는 실패 시 promise만
  reject하고 전용 워커를 회수하지 않아 실패 N번이 워커 N개로 쌓였다
- getOutlineItems: getOutline 거절을 삼키고 문서 교체를 감지해 빈 목록 반환.
  안 막으면 destroy가 유발한 거절이 **이전 로드의 catch**로 흘러가 방금 열린
  새 문서 위에 오류 화면을 띄운다 (남은 stale-load 문제는 #37)
- #resolveDestPage가 doc을 명시로 받는다 — 교체 후 옛 dest를 새 문서에 풀지 않는다
- downloadCurrentPdf·jumpToOutline: destroy가 노출시키는 거절을 삼킨다
- workerPort를 쓰면 안 되는 이유를 주석으로 고정 (공유 워커면 cross-document kill)

Closes #35

typecheck 0 · vitest 35/35 (7 files) · build 0. pdf-host.test.ts는 이 repo 첫
vi.mock 사용이며, src를 되돌리면 6개 중 4개가 실패함을 확인했다.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@onetwothr1
onetwothr1 requested a review from enu3379 as a code owner July 28, 2026 07:22
@coderabbitai

coderabbitai Bot commented Jul 28, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

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

Next review available in: 45 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

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: 7fee5e24-a04d-45c0-9069-39e5bca3c03c

📥 Commits

Reviewing files that changed from the base of the PR and between e2884d4 and 8e96f41.

📒 Files selected for processing (2)
  • src/viewer/pdf-host.ts
  • test/pdf-host.test.ts
📝 Walkthrough

Walkthrough

PdfHost의 로드 경쟁 상태와 실패 정리를 보강하고, 문서 교체 후 이전 문서를 해제하도록 변경했다. 목차 조회를 문서 스냅샷에 한정하고, 다운로드·목차 이동의 Promise 거절과 최신 로드 오류를 처리했다. 테스트와 관련 문서를 갱신했다.

Changes

문서 수명주기 및 비동기 안전성

Layer / File(s) Summary
문서 로드와 해제 흐름
src/viewer/pdf-host.ts, test/pdf-host.test.ts, docs/fig-extract-integration.md
로드 순번으로 이전 요청을 식별하고, 실패한 loadingTask와 교체된 문서를 정리하며 새 문서 전환 후 이전 문서를 destroy한다. 관련 전환 순서, 실패, 재사용 동작을 테스트한다.
문서 교체 중 목차 탐색 보호
src/viewer/pdf-host.ts, src/viewer/main.ts, test/pdf-host.test.ts
목차 추출을 문서 스냅샷 기준으로 수행하고, 문서 교체·조회 실패 시 빈 결과를 반환하며 목적지 이동 거절을 처리한다.
다운로드와 최신 로드 오류 처리
src/viewer/main.ts
문서 데이터 다운로드 실패를 조용히 종료하고, superseded 로드의 후속 오류 처리를 건너뛴다.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related issues

Suggested reviewers: enu3379

Sequence Diagram(s)

sequenceDiagram
  participant Viewer
  participant PdfHost
  participant loadingTask
  participant PDFDocumentProxy
  Viewer->>PdfHost: loadFile 또는 loadUrl
  PdfHost->>loadingTask: await promise
  loadingTask-->>PdfHost: 문서 또는 superseded 오류
  PdfHost->>Viewer: 새 문서 설정
  PdfHost->>PDFDocumentProxy: 이전 문서 destroy
  PdfHost->>loadingTask: 실패 태스크 destroy
Loading

Poem

토끼가 새 문서를 열어요
헌 문서는 조용히 잠들고
목차 길은 엉키지 않아요
실패한 약속은 살짝 흘려보내요
당근처럼 깔끔한 전환이에요 🐇

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 79.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed 제목이 문서 교체 시 이전 PDFDocumentProxy를 destroy하도록 한 핵심 변경을 정확하고 간결하게 요약합니다.
Linked Issues check ✅ Passed #35의 요구인 교체 후 이전 문서 destroy, 안전한 정리 순서, 지연 비동기 작업 처리, 반복 교체 테스트를 충족합니다.
Out of Scope Changes check ✅ Passed 문서 수정과 테스트 추가는 문서 수명 주기 수정과 직접 연관되어 있어 명확한 범위 이탈로 보이지 않습니다.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/35-destroy-previous-pdf-document

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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/viewer/pdf-host.ts (1)

100-120: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

동시 로드 결과를 최신 요청에 맞춰 정리해야 합니다.

loadUrl/loadFileawait #awaitLoad() 완료 순서로 #setDocument(previous)->destroy()를 실행합니다. 느린 URL 로드(A) 진행 중 사용자가 새 파일을 열면(B), B가 먼저 적용된 뒤 A가 늦게 완료되면 #setDocument(A)가 현재 표시 중인 B를 파괴해 사용자가 마지막으로 요청한 문서에 대한 참조가 사라집니다. 완료 시점에 최신 시퀀스인지 확인하고, 그렇지 않으면 문서만 destroy()하는 대신 다시 설정하지 않도록 해야 합니다.

🤖 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 `@src/viewer/pdf-host.ts` around lines 100 - 120, Update loadUrl and loadFile
to assign each load a monotonically increasing request sequence before awaiting
`#awaitLoad`. After loading completes, apply `#setDocument` only when that sequence
is still the latest; otherwise destroy the stale loaded document directly
without replacing the currently displayed document.
🧹 Nitpick comments (2)
src/viewer/pdf-host.ts (1)

122-134: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

loadingTask 매개변수에 pdfjs-dist의 PDFDocumentLoadingTask 타입 재사용을 고려.

현재 { promise: Promise<PDFDocumentProxy>; destroy(): Promise<void> } 형태의 최소 구조적 타입을 직접 선언하고 있습니다. pdfjs-dist가 이미 PDFDocumentLoadingTask 타입을 display/api에서 export하므로(테스트 파일에서 PDFDocumentProxy를 같은 경로에서 import하는 것과 동일하게 재사용 가능), 이를 사용하면 향후 pdf.js API 변경 시 타입 드리프트를 줄일 수 있습니다.

-async `#awaitLoad`(loadingTask: { promise: Promise<PDFDocumentProxy>; destroy(): Promise<void> }) {
+async `#awaitLoad`(loadingTask: PDFDocumentLoadingTask) {
🤖 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 `@src/viewer/pdf-host.ts` around lines 122 - 134, Update the `#awaitLoad` method
parameter to use pdfjs-dist’s exported PDFDocumentLoadingTask type from the same
display/api import source, replacing the locally declared structural type while
preserving the existing promise handling and destroy-on-error behavior.
test/pdf-host.test.ts (1)

63-177: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

getOutlineItems의 문서 교체 중단 로직에 대한 테스트 보완 제안.

이번 PR의 핵심 변경 중 하나인 getOutlineItems(문서 교체 중 오래된 outline 결과를 빈 배열로 차단하는 로직, pdf-host.ts 220-258행)에 대한 테스트가 이 스위트에 없습니다. doc.getOutline()이 아직 resolve되지 않은 상태에서 load()로 새 문서를 교체한 뒤, 이전 호출의 반환값이 []인지 검증하는 테스트를 추가하면 회귀를 방지할 수 있습니다.

it('drops outline results from a document that was replaced mid-fetch (`#35`)', async () => {
  const host = makeHost();
  const first = makeDoc('first');
  await load(host, first);

  let resolveOutline: (v: unknown) => void;
  (first as unknown as { getOutline: () => Promise<unknown> }).getOutline = () =>
    new Promise((resolve) => { resolveOutline = resolve; });

  const pending = host.getOutlineItems();
  await load(host, makeDoc('second'));
  resolveOutline!([{ title: 'A', dest: null, items: [] }]);

  expect(await pending).toEqual([]);
});
🤖 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 `@test/pdf-host.test.ts` around lines 63 - 177, Extend the PdfHost document
lifecycle suite with a test for getOutlineItems: start an unresolved outline
fetch on the first document, replace it via load with a second document, then
resolve the original fetch and assert that the pending call returns an empty
array. Use the existing makeHost, makeDoc, and load helpers and preserve the
current lifecycle assertions.
🤖 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.

Outside diff comments:
In `@src/viewer/pdf-host.ts`:
- Around line 100-120: Update loadUrl and loadFile to assign each load a
monotonically increasing request sequence before awaiting `#awaitLoad`. After
loading completes, apply `#setDocument` only when that sequence is still the
latest; otherwise destroy the stale loaded document directly without replacing
the currently displayed document.

---

Nitpick comments:
In `@src/viewer/pdf-host.ts`:
- Around line 122-134: Update the `#awaitLoad` method parameter to use
pdfjs-dist’s exported PDFDocumentLoadingTask type from the same display/api
import source, replacing the locally declared structural type while preserving
the existing promise handling and destroy-on-error behavior.

In `@test/pdf-host.test.ts`:
- Around line 63-177: Extend the PdfHost document lifecycle suite with a test
for getOutlineItems: start an unresolved outline fetch on the first document,
replace it via load with a second document, then resolve the original fetch and
assert that the pending call returns an empty array. Use the existing makeHost,
makeDoc, and load helpers and preserve the current lifecycle assertions.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: dab972e5-23f8-4928-aab0-c9628ed9741b

📥 Commits

Reviewing files that changed from the base of the PR and between 8bbc8ab and a0dae7f.

📒 Files selected for processing (4)
  • docs/fig-extract-integration.md
  • src/viewer/main.ts
  • src/viewer/pdf-host.ts
  • test/pdf-host.test.ts

이 가드는 #35에서 가장 위험한 지점(destroy가 유발한 거절이 이전 로드의 catch로
흘러가 새 문서 위에 오류 화면을 띄움)을 막는데, 수동 QA로만 확인 가능했다.
로컬 파일은 getOutline이 밀리초에 끝나 경합 창이 사실상 0이라 재현되지 않고,
원격 URL + 네트워크 스로틀링이 필요했다.

- 조회 실패가 예외로 새어나가지 않고 빈 목차가 된다
- 조회 중 문서가 교체되면 옛 목차로 새 화면을 덮지 않는다
- happy path(중첩 목차 평탄화 + dest→page 해석) — #resolveDestPage가 doc을
  명시로 받도록 바꾼 것의 회귀 가드

dev 기준으로 되돌리면 9개 중 6개가 실패함을 확인했다 (나머지 3개는 수정 전에도
통과가 맞는 회귀 가드다). typecheck 0 · vitest 38/38 · build 0

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@onetwothr1

Copy link
Copy Markdown
Collaborator Author

수동 확인 결과 (완료)

# 항목 결과
1 정상 교체 ✅ 논문 12편 연속 교체, 오류·이상 없음
2 스캔 중 교체 ✅ 실패 메시지 없이 새 문서 카드 정상 렌더
3 목차 로드 중 교체 ⚠️ 로컬 파일로는 경합 창이 사실상 0 — 아래 참고
4 누수 ✅ 12편 교체 + GC 후 JS 힙 15.4MB → 16.9MB
5 실패 로드 반복 ✅ 없는 원격 URL 반복 로드 후 pdf.worker-*.mjs 0개

3번은 단위 테스트로 대체했다 (1599a0b). 로컬 파일은 getDocument({data})로 전체 버퍼를 넘겨 getOutline()이 밀리초에 끝나므로, 수동으로는 그 경합을 재현할 수 없다(원격 URL + 네트워크 스로틀링이 필요). 대신 세 케이스를 코드로 고정했다:

  • 조회 실패가 예외로 새어나가지 않고 빈 목차가 된다
  • 조회 중 문서가 교체되면 옛 목차로 새 화면을 덮지 않는다
  • happy path(중첩 목차 평탄화 + dest→page) — #resolveDestPagedoc을 명시로 받도록 바꾼 것의 회귀 가드

dev 기준으로 되돌리면 9개 중 6개가 실패함을 확인했다(나머지 3개는 수정 전에도 통과가 맞는 회귀 가드).

최종: typecheck 0 · vitest 38/38 (7 파일) · build 0

CodeRabbit 지적. 느린 URL 로드(A) 중에 파일(B)을 드롭하면 B가 먼저 적용되는데,
그 뒤 A가 완료되면 #setDocument(A)가 화면에 떠 있는 B를 destroy한다.

이 겹침 자체는 선재 결함이지만(#37), 이전에는 "옛 문서가 화면을 덮어씀"이라는
표시 문제였던 것이 이 PR의 destroy 때문에 "사용자가 연 문서가 파괴됨"으로
승격된다. FiguresTab이 B를 스캔 중이면 abort되지 않은 채 파괴된 문서를 만나
오류 UI까지 뜬다. 그래서 destroy를 추가한 이 PR에서 함께 막는다.

- PdfHost가 로드마다 일련번호를 부여하고, 완료 시점에 최신이 아니면 방금 받은
  문서를 스스로 destroy한 뒤 PdfLoadSupersededError로 물러난다
- main.ts의 두 catch가 이 신호를 오류로 취급하지 않고 조용히 반환한다 —
  화면은 더 최신 로드의 것이므로 건드리면 안 된다

표시 상태·TOC·docData의 stale 갱신은 main.ts 레벨 문제라 #37에 남겨 둔다.

typecheck 0 · vitest 39/39 · build 0

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@onetwothr1

Copy link
Copy Markdown
Collaborator Author

CodeRabbit 지적 반영 — e2884d4

느린 URL 로드(A) 진행 중 사용자가 새 파일을 열면(B), B가 먼저 적용된 뒤 A가 늦게 완료되면 #setDocument(A)가 현재 표시 중인 B를 파괴해 사용자가 마지막으로 요청한 문서에 대한 참조가 사라집니다.

타당한 지적이라 수정했다. 겹친 로드 자체는 선재 결함(#37)이지만, 이 PR이 심각도를 올린다는 게 핵심이다:

수정 전 이 PR (지적 반영 전)
늦게 끝난 로드 옛 문서가 화면을 덮어씀 (표시 문제) 사용자가 연 문서가 파괴됨
FiguresTab B를 계속 스캔 B가 파괴됨 → abort된 적 없으므로 signal.aborted가 false → 오류 UI까지 표시

그래서 destroy를 추가한 이 PR에서 함께 막았다.

변경

  • PdfHost가 로드마다 일련번호(#loadSeq)를 부여하고, 완료 시점에 최신이 아니면 방금 받은 자기 문서를 destroy한 뒤 PdfLoadSupersededError로 물러난다. 현재 표시 중인 문서는 건드리지 않는다.
  • main.ts의 두 catch가 이 신호를 오류로 취급하지 않고 조용히 반환한다. 일반 실패로 처리하면 정상 로드된 새 문서 위에 오류 화면이 뜨므로, 가드를 figuresTab.setDocument(null)보다 앞에 뒀다.

검증

신규 테스트 discards a late load instead of destroying the document the user actually opened (#35) — 느린 URL 로드 중 파일 드롭을 재현해, 늦게 온 문서만 destroy되고 pdfDocument·뷰어가 사용자가 연 문서 그대로임을 고정한다.

dev 기준으로 되돌리면 10개 중 7개 실패 (나머지 3개는 수정 전에도 통과가 맞는 회귀 가드). typecheck 0 · vitest 39/39 · build 0.

범위 밖으로 남긴 것

표시 상태·TOC·docData의 stale 갱신은 main.ts 레벨의 로드 epoch 문제라 #37에 그대로 있다. 이번 수정은 파괴만 막는다 — 늦게 끝난 로드가 화면을 덮어쓰는 표시 문제는 여전히 #37의 몫이다.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
src/viewer/main.ts (1)

150-157: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

문서 교체 오류만 무시하고 실제 오류는 전파해야 합니다.

두 경로 모두 모든 Promise 거절을 삼켜 네트워크, 권한, 잘못된 destination 같은 사용자 조치가 필요한 오류를 숨깁니다.

  • src/viewer/main.ts#L150-L157: 현재 문서가 교체된 경우에만 다운로드 거절을 무시하고, 그 외 오류는 기존 오류 표시 경로로 전달하세요.
  • src/viewer/main.ts#L901-L903: stale 문서 거절만 필터링하고 목차 이동의 실제 오류는 사용자에게 표시하거나 전파하세요.
🤖 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 `@src/viewer/main.ts` around lines 150 - 157, Update the download handling
around doc.getData at src/viewer/main.ts lines 150-157 and the table-of-contents
navigation handling at src/viewer/main.ts lines 901-903 so only stale-document
rejections caused by replacing or destroying the current PDF are ignored;
rethrow or route all other errors through the existing user-facing error path,
preserving actual network, permission, destination, and other failures.
src/viewer/pdf-host.ts (1)

252-290: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

stale 목차 결과를 []로 반환하면 새 문서의 목차를 지울 수 있습니다.

문서 교체 중 getOutline()이 거절되면 getOutlineItems()[]를 반환하고, src/viewer/main.tsrenderToc(await host.getOutlineItems())가 이를 정상적인 “목차 없음”으로 렌더링합니다. 따라서 이전 문서 A의 목차 요청이 새 문서 B 전환 후 거절되면 A의 로드 흐름이 B 화면의 목차를 빈 상태로 덮어쓸 수 있습니다.

stale 결과와 실제 빈 목차를 구분하도록 superseded 오류를 전파하거나 { items, stale } 같은 명시적 결과를 반환하고, main.ts에서 stale 결과는 렌더링하지 않도록 수정해야 합니다.

수정 방향
} catch {
+  if (this.#doc !== doc) throw new PdfLoadSupersededError();
   return [];
}

-    if (this.#doc !== doc) return;
+    if (this.#doc !== doc) throw new PdfLoadSupersededError();

-    return this.#doc === doc ? items : [];
+    if (this.#doc !== doc) throw new PdfLoadSupersededError();
+    return items;
🤖 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 `@src/viewer/pdf-host.ts` around lines 252 - 290, Update getOutlineItems so a
superseded document request is distinguishable from a genuinely empty outline:
do not return [] when getOutline rejects or when this.#doc changes, and instead
propagate a superseded signal through the existing API (for example, an explicit
stale result). Update the caller in main.ts around renderToc(await
host.getOutlineItems()) to skip rendering stale results while preserving [] for
documents that genuinely have no outline.
🤖 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 `@src/viewer/pdf-host.ts`:
- Around line 119-137: Update loadUrl and loadFile failure handling around
`#awaitLoad` so a failed request whose seq differs from this.#loadSeq throws
PdfLoadSupersededError instead of propagating the ordinary load error. Preserve
normal error propagation for the current request and ensure stale failures
cannot clear or replace the document established by a newer load.

---

Outside diff comments:
In `@src/viewer/main.ts`:
- Around line 150-157: Update the download handling around doc.getData at
src/viewer/main.ts lines 150-157 and the table-of-contents navigation handling
at src/viewer/main.ts lines 901-903 so only stale-document rejections caused by
replacing or destroying the current PDF are ignored; rethrow or route all other
errors through the existing user-facing error path, preserving actual network,
permission, destination, and other failures.

In `@src/viewer/pdf-host.ts`:
- Around line 252-290: Update getOutlineItems so a superseded document request
is distinguishable from a genuinely empty outline: do not return [] when
getOutline rejects or when this.#doc changes, and instead propagate a superseded
signal through the existing API (for example, an explicit stale result). Update
the caller in main.ts around renderToc(await host.getOutlineItems()) to skip
rendering stale results while preserving [] for documents that genuinely have no
outline.
🪄 Autofix (Beta)

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: 82e787a8-c5b0-4ed7-840f-5cecc903ffb4

📥 Commits

Reviewing files that changed from the base of the PR and between 1599a0b and e2884d4.

📒 Files selected for processing (3)
  • src/viewer/main.ts
  • src/viewer/pdf-host.ts
  • test/pdf-host.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • test/pdf-host.test.ts

Comment thread src/viewer/pdf-host.ts
CodeRabbit 후속 지적. #adopt는 성공 경로만 막았고 실패 경로는 그대로 원본 오류를
올려보냈다. B가 먼저 성공한 뒤 A(seq 1)가 실패하면 main.ts의 catch가 일반
오류로 처리해 figuresTab.setDocument(null) + setError()를 실행하고, 그 화면에는
사용자가 실제로 연 B가 떠 있다.

#awaitLoad가 seq를 받아, 실패 시에도 최신이 아니면 PdfLoadSupersededError로
바꿔 던진다. loadingTask 정리는 두 경우 모두 그대로 수행한다.

밀려나지 않은 로드의 실패는 원본 오류를 유지해야 오류 화면이 정상 동작하므로
그 회귀 가드도 함께 넣었다.

typecheck 0 · vitest 41/41 · build 0

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@onetwothr1

Copy link
Copy Markdown
Collaborator Author

CodeRabbit 후속 지적 반영 — 8e96f41

B 요청이 먼저 성공한 뒤에 A(seq 1)가 실패하면 #adopt가 실행되기 전에 #awaitLoad가 setError(null) 이상을 호출하므로 현재 문서가 사라집니다.

맞다. 앞선 수정(e2884d4)이 성공 경로만 막고 실패 경로를 빠뜨렸다.

#adopt는 로드가 성공했을 때만 호출된다. A가 실패하면 #awaitLoad가 원본 오류를 그대로 올려보내고, main.tscatch는 그걸 일반 오류로 처리해 figuresTab.setDocument(null)setError()/showMissingFileState()를 실행한다. 그 화면에는 사용자가 실제로 연 B가 떠 있으므로, B의 그림·표 탭이 죽고 B 위에 오류 화면이 덮인다.

변경

#awaitLoadseq를 받아 실패 시에도 최신 여부를 판정한다 — 밀려났으면 PdfLoadSupersededError로 바꿔 던진다. loadingTask 정리(워커 회수)는 두 경우 모두 그대로 수행한다.

성공/실패 두 경로가 이제 같은 판정을 쓴다:

로드 결과 \ 상태 최신 밀려남
성공 #adopt → 채택 방금 받은 문서 destroy → superseded
실패 원본 오류 그대로 task destroy → superseded

검증

테스트 2건 추가:

  • reports a late load FAILURE as superseded, not as an error (#35) — B 성공 후 A 실패 시 PdfLoadSupersededError로 나오고 loadingTask가 정리되며 B는 무사한지
  • still reports a current load failure as a real error (#35)회귀 가드. 밀려나지 않은 로드의 실패는 원본 오류를 유지해야 한다. 안 그러면 오류 화면이 영영 안 뜬다

dev 기준으로 되돌리면 12개 중 8개 실패. typecheck 0 · vitest 41/41 (7 파일) · build 0.

@onetwothr1
onetwothr1 merged commit 2ef8da7 into dev Jul 28, 2026
3 checks passed
@onetwothr1
onetwothr1 deleted the fix/35-destroy-previous-pdf-document branch July 28, 2026 13: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.

fix: 문서를 교체해도 이전 PDFDocumentProxy를 destroy하지 않아 세션 내내 상주한다

1 participant