[OGL-1376] feat(etno): add pdf media viewer#17
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughA new PDF viewer feature is implemented using the Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant DetailView as DetailView Component
participant PdfViewer as PdfViewer Component
participant EmbedPDF as `@embedpdf` Engine
participant Plugins as Plugins<br/>(Scroll, Zoom, Render)
participant Toolbar as PdfViewerToolbar
User->>DetailView: Navigate to Detail with PDF media
DetailView->>PdfViewer: Mount with detail prop containing PDF URL
PdfViewer->>EmbedPDF: Initialize engine and register plugins
PdfViewer->>EmbedPDF: Initialize DocumentContent with activeDocumentId
EmbedPDF->>Plugins: Load plugins (document-manager, scroll, viewport, zoom)
Note over PdfViewer,Plugins: Loading State
PdfViewer->>User: Display "Loading PDF..." message
Note over EmbedPDF,Plugins: Engine Ready
EmbedPDF->>PdfViewer: Engine and plugins ready
PdfViewer->>Plugins: Query page dimensions and states
PdfViewer->>User: Render Viewport with Scroller and RenderLayers per page
PdfViewer->>Toolbar: Mount with documentId reference
User->>Toolbar: Click zoom in/out or page navigation
Toolbar->>Plugins: Call zoom?.zoomIn(), scroll?.scrollToPreviousPage(), etc.
Plugins->>EmbedPDF: Update rendering state
EmbedPDF->>PdfViewer: Update RenderLayers with new page content
PdfViewer->>User: Display updated PDF view
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/etno/src/components/DetailViewers/PdfViewer/PdfViewer.vue`:
- Around line 20-44: Wrap the duplicated v-if and repeated prop by replacing the
separate v-if="isLoaded" on PdfViewerToolbar and Viewport with a single
<template v-if="isLoaded"> that contains both PdfViewerToolbar and Viewport so
they mount/unmount together; inside that template pass
:document-id="activeDocumentId" to both children (keep the existing :page-index
on RenderLayer and :document-id on Scroller) and remove the individual v-if and
duplicate prop on PdfViewerToolbar and Viewport to avoid repetition.
- Around line 67-84: The current pdfUrl and plugins are computed once in setup
and use a hardcoded fallback; change pdfUrl to a computed (or ref updated via
watch) that derives from props.detail?.media?.documents?.[0]?.url so it updates
when detail is populated, remove the external fallback URL entirely, and make
plugins recompute whenever that computed pdfUrl changes (e.g. create plugins
inside a computed or rebuild them in a watch) so DocumentManagerPluginPackage /
createPluginRegistration and the initialDocuments entry use the live url; also
update the template to render an explicit empty/error state when the computed
pdfUrl is falsy instead of loading an external fallback.
In `@apps/etno/src/components/DetailViewers/PdfViewer/PdfViewerToolbar.vue`:
- Around line 2-4: The toolbar's root <div> in PdfViewerToolbar.vue is missing
relative positioning so the absolutely positioned zoom controls ("Zoom Controls"
div with classes absolute left-1/2 -translate-x-1/2) can escape and anchor to an
ancestor; fix by adding the "relative" class to the root toolbar container (the
top-level div in PdfViewerToolbar.vue that currently has bg-white border-b px-4
py-2 flex items-center justify-end) so the centered zoom group is positioned
relative to the toolbar.
- Line 26: The navigation buttons in PdfViewerToolbar.vue currently check only
scrollState.currentPage which can flicker before the document loads; update the
:disabled bindings for both previous and next controls to also require
scrollState.totalPages > 0 (e.g., :disabled="scrollState.totalPages <= 0 ||
scrollState.currentPage <= 1" for Prev and :disabled="scrollState.totalPages <=
0 || scrollState.currentPage >= scrollState.totalPages" for Next) so navigation
stays disabled until totalPages is initialized; adjust the bindings referencing
scrollState.currentPage and scrollState.totalPages in the template (the
:disabled attributes) accordingly.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: d6a1bbe9-3bcb-4d31-a92a-46b847e139db
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
apps/etno/package.jsonapps/etno/src/components/DetailViewers/BaseViewer.vueapps/etno/src/components/DetailViewers/PdfViewer.vueapps/etno/src/components/DetailViewers/PdfViewer/PdfViewer.vueapps/etno/src/components/DetailViewers/PdfViewer/PdfViewerToolbar.vueapps/etno/src/i18n/en.jsonapps/etno/src/i18n/sk.jsonapps/etno/src/views/DetailView.vuepackages/components/src/components/atoms/BaseIcon/BaseIcon.vue
💤 Files with no reviewable changes (1)
- apps/etno/src/components/DetailViewers/PdfViewer.vue
56b52bc to
d210100
Compare
d210100 to
b4a5fe5
Compare
b4a5fe5 to
568436f
Compare
Summary by CodeRabbit
Release Notes
New Features
Style
Chores