Conversation
Reviewer's GuideAdds an optional thumbnail sidebar to the PdfReader component, wires it into the pdf.js event lifecycle, adjusts scaling controls, and introduces new options and callbacks for page initialization and load completion. Sequence diagram for PdfReader pagesloaded and thumbnail creationsequenceDiagram
actor User
participant BlazorPdfReader as PdfReader
participant JsPdfModule as PdfReader_razor_js
participant PdfJsViewer as pdfjs_PDFViewer
participant EventBus as pdfjs_EventBus
User->>BlazorPdfReader: Navigate to page with PdfReader
BlazorPdfReader->>JsPdfModule: Initialize(Options.Url, EnableThumbnails, TriggerPagesInit, TriggerPagesLoaded, ...)
JsPdfModule->>PdfJsViewer: loadDocument(Options.Url)
PdfJsViewer-->>EventBus: pagesloaded(pagesCount)
EventBus-->>JsPdfModule: pagesloaded event
alt options.enableThumbnails == true
JsPdfModule->>JsPdfModule: query .bb-view-thumbnails
loop for each page i
JsPdfModule->>PdfJsViewer: pdfDocument.getPage(i + 1)
PdfJsViewer-->>JsPdfModule: page
JsPdfModule->>JsPdfModule: makeThumb(page)
JsPdfModule->>JsPdfModule: create thumbnail item + img
JsPdfModule->>JsPdfModule: append to .bb-view-thumbnails
end
JsPdfModule->>JsPdfModule: wire click handler on .bb-view-thumbnail-item
end
alt options.triggerPagesLoaded == true
JsPdfModule->>BlazorPdfReader: invokeMethodAsync PagesLoaded(pagesCount)
BlazorPdfReader->>BlazorPdfReader: PagesLoaded(int pagesCount)
alt Options.OnPagesLoadedAsync != null
BlazorPdfReader->>BlazorPdfReader: await Options.OnPagesLoadedAsync(pagesCount)
end
end
Sequence diagram for thumbnail click and active page synchronizationsequenceDiagram
actor User
participant Thumbnails as ThumbnailsSidebar
participant JsPdfModule as PdfReader_razor_js
participant PdfJsViewer as pdfjs_PDFViewer
participant EventBus as pdfjs_EventBus
participant BlazorPdfReader
User->>Thumbnails: Click .bb-view-thumbnail-item
Thumbnails->>JsPdfModule: click event (delegated)
JsPdfModule->>JsPdfModule: remove .active from current thumbnail
JsPdfModule->>JsPdfModule: add .active to clicked item
JsPdfModule->>JsPdfModule: read data-bb-page as index
JsPdfModule->>PdfJsViewer: set currentPageNumber = index
PdfJsViewer-->>EventBus: pagechanging(pageNumber)
EventBus-->>JsPdfModule: pagechanging event
JsPdfModule->>JsPdfModule: update .bb-view-num value
alt options.enableThumbnails == true
JsPdfModule->>JsPdfModule: update .active thumbnail to match page
JsPdfModule->>JsPdfModule: scrollIntoView(active thumbnail)
end
alt options.triggerPageChanged == true
JsPdfModule->>BlazorPdfReader: invokeMethodAsync pageChanged(pageNumber)
end
Class diagram for updated PdfReader and PdfReaderOptionsclassDiagram
class PdfReaderOptions {
bool ShowToolbar
bool EnableThumbnails
string Url
Func~int, Task~ OnPagesInitAsync
Func~int, Task~ OnPagesLoadedAsync
Func~int, Task~ OnPageChangedAsync
Func~bool, Task~ OnTwoPagesOneViewAsync
}
class PdfReader {
PdfReaderOptions Options
string CurrentScale
void SetCurrentScale(string value)
Task OnAfterRenderAsync(bool firstRender)
Task PagesInit(int pagesCount)
Task PagesLoaded(int pagesCount)
}
PdfReader --> PdfReaderOptions : uses
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In
pagesloadedyou usepdfViewer.getPagesOverview().map(async ...)without consuming the returned promises; consider usingfor...ofwithawaitorPromise.allto avoid unhandled async side effects and to make the thumbnail creation order/determinism clearer. - In the
pagesloadedandpagechanginghandlers you callthumbnailsContainer.querySelector('.active').classList.remove('active')without checking for null; add a null guard to avoid runtime errors when no thumbnail is currently active. - The click handler on
.bb-view-barassumes.bb-view-thumbnailsexists and callsclassList.toggleunconditionally; when thumbnails are disabled this will benull, so add a check before toggling to prevent errors.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `pagesloaded` you use `pdfViewer.getPagesOverview().map(async ...)` without consuming the returned promises; consider using `for...of` with `await` or `Promise.all` to avoid unhandled async side effects and to make the thumbnail creation order/determinism clearer.
- In the `pagesloaded` and `pagechanging` handlers you call `thumbnailsContainer.querySelector('.active').classList.remove('active')` without checking for null; add a null guard to avoid runtime errors when no thumbnail is currently active.
- The click handler on `.bb-view-bar` assumes `.bb-view-thumbnails` exists and calls `classList.toggle` unconditionally; when thumbnails are disabled this will be `null`, so add a check before toggling to prevent errors.
## Individual Comments
### Comment 1
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:175-181` </location>
<code_context>
+ if (options.enableThumbnails) {
+ const thumbnailsContainer = el.querySelector(".bb-view-thumbnails");
+ if (thumbnailsContainer) {
+ const active = thumbnailsContainer.querySelector('.active');
+ active.classList.remove('active');
+
+ const item = thumbnailsContainer.querySelector(`[data-bb-page='${page}']`);
+ item.classList.add("active");
+ item.scrollIntoView({ behavior: 'smooth', block: "nearest", inline: "start" });
+ }
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard against `null` when updating the active thumbnail on page change.
In the `pagechanging` handler, both `thumbnailsContainer.querySelector('.active')` and the lookup for `[data-bb-page='${page}']` can return `null`. Without checks, `active.classList.remove('active')` and `item.classList.add('active')` will throw if no active thumbnail exists yet or no item matches the page. Please add null checks (as in the click handler) before modifying these elements.
</issue_to_address>
### Comment 2
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:228-231` </location>
<code_context>
});
}
+
+ const thumbnailsToggle = el.querySelector(".bb-view-bar");
+ if (thumbnailsToggle) {
+ EventHandler.on(thumbnailsToggle, "click", e => {
+ const thumbnailsEl = el.querySelector(".bb-view-thumbnails");
+ thumbnailsEl.classList.toggle("show");
+ });
</code_context>
<issue_to_address>
**issue (bug_risk):** Toggling thumbnails will throw when thumbnails are disabled.
The click handler is always attached to `.bb-view-bar`, but `.bb-view-thumbnails` only exists when `Options.EnableThumbnails` is true. When thumbnails are disabled, `thumbnailsEl` will be `null` and `thumbnailsEl.classList.toggle("show")` will throw. Add a null check (or avoid registering the handler when thumbnails are disabled).
</issue_to_address>
### Comment 3
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:237` </location>
<code_context>
pdfViewer.currentScaleValue = v / 100;
}
+const makeThumb = async page => {
+ const outputScale = window.devicePixelRatio || 1;
+ const vp = page.getViewport({ scale: 1 });
+ const canvas = document.createElement("canvas");
+ const scaleSize = 1;
+ canvas.width = vp.width * scaleSize * outputScale;
+ canvas.height = vp.height * scaleSize * outputScale;
+
+ await page.render({
+ canvasContext: canvas.getContext("2d"),
+ viewport: page.getViewport({ scale: scaleSize * outputScale })
+ }).promise;
+
+ return canvas;
+}
+
</code_context>
<issue_to_address>
**suggestion (performance):** Thumbnail rendering may be overkill in resolution relative to displayed size.
Here you render the thumbnail canvas at `devicePixelRatio * viewport` with `scaleSize = 1`, but CSS constrains the thumbnail to `width: 128px`. On large PDFs and high-DPI displays this can cause unnecessary memory and CPU use. Please consider basing the render scale on the intended thumbnail width (e.g., 128px * devicePixelRatio) or reducing `scaleSize` accordingly to keep quality acceptable while lowering rendering cost.
```suggestion
const updateScale = (pdfViewer, button, rate) => {
pdfViewer.currentScaleValue = v / 100;
}
const makeThumb = async page => {
const outputScale = window.devicePixelRatio || 1;
const THUMBNAIL_CSS_WIDTH = 128;
// First get an unscaled viewport to inspect the page width in CSS pixels.
const baseViewport = page.getViewport({ scale: 1 });
// Compute a scale so that the rendered thumbnail is ~THUMBNAIL_CSS_WIDTH wide in CSS pixels,
// then multiply by devicePixelRatio so the backing buffer is high-DPI without being excessive.
const targetScale = Math.min(
1,
(THUMBNAIL_CSS_WIDTH * outputScale) / baseViewport.width
);
const viewport = page.getViewport({ scale: targetScale });
const canvas = document.createElement("canvas");
canvas.width = viewport.width;
canvas.height = viewport.height;
await page.render({
canvasContext: canvas.getContext("2d"),
viewport
}).promise;
return canvas;
}
```
</issue_to_address>
### Comment 4
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:128` </location>
<code_context>
}
});
+ eventBus.on("pagesloaded", async e => {
+ if (options.enableThumbnails) {
+ const thumbnailsContainer = el.querySelector(".bb-view-thumbnails");
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting helpers for thumbnail creation/activation and scale button updates to avoid duplicated logic, repeated DOM queries, and side-effectful async map usage.
Using `map` for async side-effects, duplicating thumbnail activation logic, and inlining scale button state all increase complexity. You can keep all functionality and simplify by introducing a few small helpers and reusing DOM references.
### 1. Avoid `map` for async side-effects & extract thumbnail creation
Replace the `map(async ...)` with a simple loop and a helper to encapsulate thumbnail creation:
```js
// helper (near makeThumb)
const createThumbnailItem = async (pdfViewer, thumbnailsContainer, pageIndex) => {
const item = document.createElement("div");
item.classList.add("bb-view-thumbnail-item");
const pageNumber = pageIndex + 1;
if (pdfViewer.currentPageNumber === pageNumber) {
item.classList.add("active");
}
item.setAttribute("data-bb-page", String(pageNumber));
thumbnailsContainer.appendChild(item);
const page = await pdfViewer.pdfDocument.getPage(pageNumber);
const canvas = await makeThumb(page);
const img = document.createElement("img");
img.src = canvas.toDataURL();
item.appendChild(img);
};
```
```js
eventBus.on("pagesloaded", async e => {
if (options.enableThumbnails) {
const thumbnailsContainer = el.querySelector(".bb-view-thumbnails");
const pages = pdfViewer.getPagesOverview();
for (let i = 0; i < pages.length; i++) {
await createThumbnailItem(pdfViewer, thumbnailsContainer, i);
}
EventHandler.on(
thumbnailsContainer,
"click",
".bb-view-thumbnail-item",
e => {
const index = parseInt(e.delegateTarget.getAttribute("data-bb-page")) || 1;
updateActiveThumbnail(thumbnailsContainer, index, { scroll: false });
pdfViewer.currentPageNumber = index;
}
);
}
if (options.triggerPagesLoaded === true) {
await invoke.invokeMethodAsync("PagesLoaded", e.pagesCount);
}
});
```
### 2. Deduplicate “active thumbnail” logic
You have almost the same logic in the click handler and `pagechanging`. Centralize it:
```js
// helper (near other helpers)
const updateActiveThumbnail = (thumbnailsContainer, page, { scroll } = {}) => {
const active = thumbnailsContainer.querySelector(".active");
if (active) active.classList.remove("active");
const item = thumbnailsContainer.querySelector(`[data-bb-page='${page}']`);
if (!item) return;
item.classList.add("active");
if (scroll) {
item.scrollIntoView({ behavior: "smooth", block: "nearest", inline: "start" });
}
};
```
```js
eventBus.on("pagechanging", async evt => {
const page = evt.pageNumber;
const pageNumberEl = el.querySelector(".bb-view-num");
if (pageNumberEl) {
pageNumberEl.value = page;
}
if (options.enableThumbnails) {
const thumbnailsContainer = el.querySelector(".bb-view-thumbnails");
if (thumbnailsContainer) {
updateActiveThumbnail(thumbnailsContainer, page, { scroll: true });
}
}
if (options.triggerPageChanged === true) {
await invoke.invokeMethodAsync("pageChanged", page);
}
}, true);
```
The click handler above reuses the same helper without scroll.
### 3. Centralize scale bounds & reuse DOM references
You already query `minus`/`plus` once; reuse those instead of re-querying on every scale change and avoid magic numbers scattered around:
```js
// top-level constants
const MIN_SCALE = 25;
const MAX_SCALE = 500;
```
```js
// helper
const updateScaleButtons = (minus, plus, scale) => {
if (!minus || !plus) return;
if (scale === MIN_SCALE) {
minus.classList.add("disabled");
plus.classList.remove("disabled");
} else if (scale === MAX_SCALE) {
plus.classList.add("disabled");
minus.classList.remove("disabled");
} else {
minus.classList.remove("disabled");
plus.classList.remove("disabled");
}
};
```
```js
const minus = el.querySelector(".bb-page-minus");
const plus = el.querySelector(".bb-page-plus");
const scaleEl = el.querySelector(".bb-view-scale");
eventBus.on("scalechanging", evt => {
const scale = evt.scale * 100;
scaleEl.value = `${Math.round(scale, 0)}%`;
updateScaleButtons(minus, plus, scale);
});
```
This keeps the clamping range in one place (`MIN_SCALE`/`MAX_SCALE`) and makes the handler easier to read.
### 4. Make `makeThumb` slightly more reusable
This doesn’t change any current behavior but makes future changes simpler:
```js
const makeThumb = async (page, scaleSize = 1) => {
const outputScale = window.devicePixelRatio || 1;
const vp = page.getViewport({ scale: 1 });
const canvas = document.createElement("canvas");
canvas.width = vp.width * scaleSize * outputScale;
canvas.height = vp.height * scaleSize * outputScale;
await page.render({
canvasContext: canvas.getContext("2d"),
viewport: page.getViewport({ scale: scaleSize * outputScale })
}).promise;
return canvas;
};
```
Call sites can continue to use `makeThumb(page)` as-is.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+228
to
+231
| const thumbnailsToggle = el.querySelector(".bb-view-bar"); | ||
| if (thumbnailsToggle) { | ||
| EventHandler.on(thumbnailsToggle, "click", e => { | ||
| const thumbnailsEl = el.querySelector(".bb-view-thumbnails"); |
There was a problem hiding this comment.
issue (bug_risk): Toggling thumbnails will throw when thumbnails are disabled.
The click handler is always attached to .bb-view-bar, but .bb-view-thumbnails only exists when Options.EnableThumbnails is true. When thumbnails are disabled, thumbnailsEl will be null and thumbnailsEl.classList.toggle("show") will throw. Add a null check (or avoid registering the handler when thumbnails are disabled).
| } | ||
| } | ||
|
|
||
| const updateScale = (pdfViewer, button, rate) => { |
There was a problem hiding this comment.
suggestion (performance): Thumbnail rendering may be overkill in resolution relative to displayed size.
Here you render the thumbnail canvas at devicePixelRatio * viewport with scaleSize = 1, but CSS constrains the thumbnail to width: 128px. On large PDFs and high-DPI displays this can cause unnecessary memory and CPU use. Please consider basing the render scale on the intended thumbnail width (e.g., 128px * devicePixelRatio) or reducing scaleSize accordingly to keep quality acceptable while lowering rendering cost.
Suggested change
| const updateScale = (pdfViewer, button, rate) => { | |
| const updateScale = (pdfViewer, button, rate) => { | |
| pdfViewer.currentScaleValue = v / 100; | |
| } | |
| const makeThumb = async page => { | |
| const outputScale = window.devicePixelRatio || 1; | |
| const THUMBNAIL_CSS_WIDTH = 128; | |
| // First get an unscaled viewport to inspect the page width in CSS pixels. | |
| const baseViewport = page.getViewport({ scale: 1 }); | |
| // Compute a scale so that the rendered thumbnail is ~THUMBNAIL_CSS_WIDTH wide in CSS pixels, | |
| // then multiply by devicePixelRatio so the backing buffer is high-DPI without being excessive. | |
| const targetScale = Math.min( | |
| 1, | |
| (THUMBNAIL_CSS_WIDTH * outputScale) / baseViewport.width | |
| ); | |
| const viewport = page.getViewport({ scale: targetScale }); | |
| const canvas = document.createElement("canvas"); | |
| canvas.width = viewport.width; | |
| canvas.height = viewport.height; | |
| await page.render({ | |
| canvasContext: canvas.getContext("2d"), | |
| viewport | |
| }).promise; | |
| return canvas; | |
| } |
There was a problem hiding this comment.
Pull request overview
This PR adds thumbnail navigation functionality to the PdfReader component, allowing users to view and navigate through PDF pages using thumbnail previews. It also introduces a distinction between page initialization and page loading callbacks, and includes UI improvements for responsive design.
Key Changes
- Added
EnableThumbnailsproperty (default: true) to show/hide thumbnail sidebar with page previews - Renamed
OnInitAsynctoOnPagesInitAsyncand added newOnPagesLoadedAsynccallback to differentiate initialization stages - Refactored scale boundary checking logic from
scale()function toscalechangingevent handler for better event-driven design
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| PdfReaderOptions.cs | Added EnableThumbnails property and new callback OnPagesLoadedAsync; renamed OnInitAsync to OnPagesInitAsync (breaking change) |
| PdfReader.razor.js | Implemented thumbnail generation and navigation logic; moved scale boundary checks to event handler; added cleanup for new event handlers |
| PdfReader.razor.css | Added thumbnail sidebar styling with transition effects and responsive layout using flexbox |
| PdfReader.razor.cs | Updated callback invocations to use new property names; refactored scale validation to use switch expression |
| PdfReader.razor | Added thumbnail container element with conditional rendering; made document title responsive with d-none d-sm-block |
| BootstrapBlazor.PdfReader.csproj | Incremented version from 10.0.1-beta04 to 10.0.1-beta05 |
Comments suppressed due to low confidence (3)
src/components/BootstrapBlazor.PdfReader/PdfReaderOptions.cs:69
- Duplicate comment: The comment "页面初始化回调方法" (Page initialization callback method) at line 67 is identical to the one at line 58. This appears to be for
OnPageChangedAsync, so the comment should be updated to "页面切换回调方法" (Page changed callback method) or similar to accurately describe this callback.
/// <summary>
/// 页面初始化回调方法
/// </summary>
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:92
- Unused variable el.
const { el, pdfViewer } = Data.get(id);
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:246
- The initial value of v is unused, since it is always overwritten.
let v = 100;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+180
to
+181
| item.classList.add("active"); | ||
| item.scrollIntoView({ behavior: 'smooth', block: "nearest", inline: "start" }); |
There was a problem hiding this comment.
Potential null reference error: The code assumes the thumbnail item element exists without null checking before calling classList.add() and scrollIntoView(). If the thumbnail for the page doesn't exist yet (e.g., still loading), this will throw an error. Add a null check: if (item) { item.classList.add("active"); item.scrollIntoView(...); }
Suggested change
| item.classList.add("active"); | |
| item.scrollIntoView({ behavior: 'smooth', block: "nearest", inline: "start" }); | |
| if (item) { | |
| item.classList.add("active"); | |
| item.scrollIntoView({ behavior: 'smooth', block: "nearest", inline: "start" }); | |
| } |
Comment on lines
+201
to
+204
| if (scale === 25) { | ||
| minus.classList.add("disabled"); | ||
| } | ||
| else if (scale === 500) { |
There was a problem hiding this comment.
Floating-point comparison issue: Comparing scale === 25 and scale === 500 with exact equality is unreliable for floating-point numbers. Due to precision issues with multiplication (e.g., evt.scale * 100), the comparison may fail to match even when the value is logically 25 or 500. Use threshold checks instead: if (scale <= 25) and if (scale >= 500) to ensure the buttons are properly disabled at the boundaries.
Suggested change
| if (scale === 25) { | |
| minus.classList.add("disabled"); | |
| } | |
| else if (scale === 500) { | |
| if (scale <= 25) { | |
| minus.classList.add("disabled"); | |
| } | |
| else if (scale >= 500) { |
| if (thumbnailsToggle) { | ||
| EventHandler.on(thumbnailsToggle, "click", e => { | ||
| const thumbnailsEl = el.querySelector(".bb-view-thumbnails"); | ||
| thumbnailsEl.classList.toggle("show"); |
There was a problem hiding this comment.
Potential null reference error: The code assumes thumbnailsEl exists without null checking before calling classList.toggle(). If EnableThumbnails is false or the element doesn't exist for any reason, this will throw an error. Add a null check: if (thumbnailsEl) { thumbnailsEl.classList.toggle("show"); }
Suggested change
| thumbnailsEl.classList.toggle("show"); | |
| if (thumbnailsEl) { | |
| thumbnailsEl.classList.toggle("show"); | |
| } |
Comment on lines
+131
to
+158
| pdfViewer.getPagesOverview().map(async (p, i) => { | ||
| const item = document.createElement("div"); | ||
| item.classList.add("bb-view-thumbnail-item"); | ||
| if (pdfViewer.currentPageNumber === i + 1) { | ||
| item.classList.add("active"); | ||
| } | ||
| item.setAttribute("data-bb-page", `${i + 1}`); | ||
| thumbnailsContainer.appendChild(item); | ||
|
|
||
| const page = await pdfViewer.pdfDocument.getPage(i + 1); | ||
| const canvas = await makeThumb(page); | ||
| const img = document.createElement("img"); | ||
| img.src = canvas.toDataURL(); | ||
| item.appendChild(img); | ||
| }); | ||
|
|
||
| EventHandler.on(thumbnailsContainer, "click", ".bb-view-thumbnail-item", e => { | ||
| const active = thumbnailsContainer.querySelector('.active'); | ||
| if (active) { | ||
| active.classList.remove('active'); | ||
| } | ||
|
|
||
| const item = e.delegateTarget; | ||
| item.classList.add("active"); | ||
|
|
||
| const index = parseInt(item.getAttribute("data-bb-page")) || 1; | ||
| pdfViewer.currentPageNumber = index; | ||
| }) |
There was a problem hiding this comment.
Missing null check: thumbnailsContainer could be null if the element doesn't exist in the DOM. Add a null check before attempting to use it: if (thumbnailsContainer) { ... } to prevent potential runtime errors.
Suggested change
| pdfViewer.getPagesOverview().map(async (p, i) => { | |
| const item = document.createElement("div"); | |
| item.classList.add("bb-view-thumbnail-item"); | |
| if (pdfViewer.currentPageNumber === i + 1) { | |
| item.classList.add("active"); | |
| } | |
| item.setAttribute("data-bb-page", `${i + 1}`); | |
| thumbnailsContainer.appendChild(item); | |
| const page = await pdfViewer.pdfDocument.getPage(i + 1); | |
| const canvas = await makeThumb(page); | |
| const img = document.createElement("img"); | |
| img.src = canvas.toDataURL(); | |
| item.appendChild(img); | |
| }); | |
| EventHandler.on(thumbnailsContainer, "click", ".bb-view-thumbnail-item", e => { | |
| const active = thumbnailsContainer.querySelector('.active'); | |
| if (active) { | |
| active.classList.remove('active'); | |
| } | |
| const item = e.delegateTarget; | |
| item.classList.add("active"); | |
| const index = parseInt(item.getAttribute("data-bb-page")) || 1; | |
| pdfViewer.currentPageNumber = index; | |
| }) | |
| if (thumbnailsContainer) { | |
| pdfViewer.getPagesOverview().map(async (p, i) => { | |
| const item = document.createElement("div"); | |
| item.classList.add("bb-view-thumbnail-item"); | |
| if (pdfViewer.currentPageNumber === i + 1) { | |
| item.classList.add("active"); | |
| } | |
| item.setAttribute("data-bb-page", `${i + 1}`); | |
| thumbnailsContainer.appendChild(item); | |
| const page = await pdfViewer.pdfDocument.getPage(i + 1); | |
| const canvas = await makeThumb(page); | |
| const img = document.createElement("img"); | |
| img.src = canvas.toDataURL(); | |
| item.appendChild(img); | |
| }); | |
| EventHandler.on(thumbnailsContainer, "click", ".bb-view-thumbnail-item", e => { | |
| const active = thumbnailsContainer.querySelector('.active'); | |
| if (active) { | |
| active.classList.remove('active'); | |
| } | |
| const item = e.delegateTarget; | |
| item.classList.add("active"); | |
| const index = parseInt(item.getAttribute("data-bb-page")) || 1; | |
| pdfViewer.currentPageNumber = index; | |
| }) | |
| } |
Comment on lines
+131
to
+145
| pdfViewer.getPagesOverview().map(async (p, i) => { | ||
| const item = document.createElement("div"); | ||
| item.classList.add("bb-view-thumbnail-item"); | ||
| if (pdfViewer.currentPageNumber === i + 1) { | ||
| item.classList.add("active"); | ||
| } | ||
| item.setAttribute("data-bb-page", `${i + 1}`); | ||
| thumbnailsContainer.appendChild(item); | ||
|
|
||
| const page = await pdfViewer.pdfDocument.getPage(i + 1); | ||
| const canvas = await makeThumb(page); | ||
| const img = document.createElement("img"); | ||
| img.src = canvas.toDataURL(); | ||
| item.appendChild(img); | ||
| }); |
There was a problem hiding this comment.
Performance issue: Using .map() with async callbacks doesn't wait for the promises to complete. This can cause race conditions where thumbnails are added to the DOM out of order or the event handler is registered before all thumbnails are rendered. Use await Promise.all(pdfViewer.getPagesOverview().map(async (p, i) => { ... })) or a for...of loop to ensure proper sequencing.
| /// 页面初始化回调方法 | ||
| /// </summary> | ||
| public Func<int, Task>? OnInitAsync { get; set; } | ||
| public Func<int, Task>? OnPagesInitAsync { get; set; } |
There was a problem hiding this comment.
Breaking change: Renaming OnInitAsync to OnPagesInitAsync will break existing code that uses this callback. Consider deprecating the old property rather than removing it immediately, or document this as a breaking change in the release notes.
| const thumbnailsContainer = el.querySelector(".bb-view-thumbnails"); | ||
| if (thumbnailsContainer) { | ||
| const active = thumbnailsContainer.querySelector('.active'); | ||
| active.classList.remove('active'); |
There was a problem hiding this comment.
Potential null reference error: The code assumes active element exists without null checking before calling classList.remove(). If no active element is found (e.g., on first page change or if thumbnails were dynamically removed), this will throw an error. Add a null check: if (active) { active.classList.remove('active'); }
Suggested change
| active.classList.remove('active'); | |
| if (active) { | |
| active.classList.remove('active'); | |
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Link issues
fixes #722
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Add optional thumbnail sidebar and related callbacks to the PdfReader component.
New Features:
Enhancements: