Skip to content

Refine desktop viewer boundary and stabilize desktop tests#39

Merged
gitgrahamdunn merged 1 commit intomainfrom
codex/update-tauri-architecture-for-multi-layer-support-b30o4p
Mar 9, 2026
Merged

Refine desktop viewer boundary and stabilize desktop tests#39
gitgrahamdunn merged 1 commit intomainfrom
codex/update-tauri-architecture-for-multi-layer-support-b30o4p

Conversation

@gitgrahamdunn
Copy link
Copy Markdown
Owner

Motivation

  • Enforce a clear renderer abstraction so UI-level viewer code does not directly depend on PDF.js and can accept future renderer adapters.
  • Surface multi-layer/scene concepts in the UI boundary while keeping renderer instantiation in infrastructure.
  • Stabilize desktop test/tooling and TypeScript checks in this environment to prevent flaky CI/test runs.

Description

  • Move PDF.js adapter instantiation into an infrastructure factory by adding createViewerRenderer in apps/desktop/src/lib/viewerRenderer.ts so only infrastructure constructs PdfjsRendererAdapter.
  • Update PdfViewer to accept an injectable rendererFactory?: () => ViewerRenderer and stop directly importing @gitplant/viewer-pdfjs, preserving the RenderScene/layer model and keeping the current single-base-layer proof path.
  • Wire App to pass createViewerRenderer into PdfViewer so the app remains desktop-ready while UI code uses only viewer-core abstractions.
  • Tighten tests and tooling: extend PdfViewer tests to assert both App.tsx and PdfViewer.tsx do not contain direct pdfjs-dist/@gitplant/viewer-pdfjs imports, switch test setup to @testing-library/jest-dom/vitest, move to vitest/config with globals, and update apps/desktop/tsconfig.json to reduce lib/type noise (skipLibCheck, lib, and test excludes).

Testing

  • Ran npm test and all desktop tests passed (4 tests across PdfViewer.test.tsx and App.test.tsx), result: success.
  • Ran npm run typecheck and TypeScript checks passed in the adjusted desktop tsconfig.json, result: success.
  • Built the web bundle with npm run build:web -w @gitplant/desktop to validate bundling of the PDF worker, result: success.
  • Attempted cargo test for Rust-side tests but the run was blocked by network access to crates.io (HTTP 403) in this environment; Rust tests were not completed due to that network constraint.

Codex Task

@gitgrahamdunn gitgrahamdunn merged commit d70f51f into main Mar 9, 2026
2 checks passed
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8989f27159

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

title: string;
overlayBytes?: Uint8Array | null;
onPageCount?: (n: number) => void;
rendererFactory?: () => ViewerRenderer;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Make renderer factory mandatory or supply a default

rendererFactory is declared optional, but when it is omitted and bytes is non-null the component now renders "Viewer renderer unavailable." instead of displaying the PDF, which is a runtime regression from the previous behavior and easy to trigger in any caller that forgets the new prop. Because the prop is optional, TypeScript will not catch those call sites; either require the prop in Props or provide an internal fallback renderer.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant