feat(PdfReader): add Document properties Function#751
Merged
Conversation
Reviewer's GuideImplements a document properties dialog for the PdfReader component, wiring pdf.js metadata into a new UI overlay, updating toolbar interactions, and making small robustness fixes to scaling and printing behavior. Sequence diagram for loading PDF metadata into the document properties dialogsequenceDiagram
participant PdfReaderInit as PdfReader_init
participant PdfJs as pdfjsLib
participant PdfDocument as pdfDocument
participant PdfViewer as pdfViewer
participant Dom as Document_DOM
PdfReaderInit->>PdfJs: getDocument(options)
PdfJs-->>PdfReaderInit: loadingTask
PdfReaderInit->>PdfJs: loadingTask.promise
PdfJs-->>PdfReaderInit: pdfDocument
PdfReaderInit->>PdfViewer: setDocument(pdfDocument)
PdfReaderInit->>PdfDocument: getMetadata()
PdfDocument-->>PdfReaderInit: metadata
PdfReaderInit->>Dom: loadMetadata(el, pdfViewer, metadata)
activate Dom
Dom->>Dom: query .bb-view-pdf-dialog-* elements
Dom->>Dom: getFilesize(metadata)
Dom-->>Dom: formatted file size
Dom->>Dom: parsePdfDate(metadata.info.CreationDate)
Dom-->>Dom: Date or null
Dom->>PdfViewer: read pagesCount
Dom->>PdfDocument: getPage(pdfViewer.currentPageNumber)
PdfDocument-->>Dom: page
Dom->>Dom: page.getViewport(scale: 1)
Dom-->>Dom: viewport (width, height)
Dom->>Dom: update dialog textContent fields
deactivate Dom
Sequence diagram for opening and closing the document properties dialogsequenceDiagram
actor User
participant Toolbar as PdfReader_toolbar
participant EventHandler as EventHandler_module
participant Dom as Document_DOM
User->>Toolbar: click .dropdown-item-doc
Toolbar->>EventHandler: delegated click handler
EventHandler->>Dom: query .bb-view-pdf-info
Dom-->>EventHandler: dialog element
EventHandler->>Dom: dialog.classList.add(show)
User->>Dom: click .btn-close-doc
Dom->>EventHandler: click handler for close button
EventHandler->>Dom: query .bb-view-pdf-info
Dom-->>EventHandler: dialog element
EventHandler->>Dom: dialog.classList.remove(show)
Flow diagram for parsing a PDF date string (parsePdfDate)flowchart TD
A["Start parsePdfDate(pdfDateString)"] --> B{"Is pdfDateString nonempty string?"}
B -- No --> Z["Return null"]
B -- Yes --> C{"Starts with D:?"}
C -- Yes --> D["Remove leading D:"]
C -- No --> E["Use original string"]
D --> F["dateStr = trimmed value"]
E --> F
F --> G{"Match dateStr against regex"}
G -- No --> Z
G -- Yes --> H["Extract year, month, day, hours, minutes, seconds, timezoneSign, timezoneHours, timezoneMinutes"]
H --> I["Create Date with local year, month-1, day, hours, minutes, seconds"]
I --> J{"timezoneSign is Z/z?"}
J -- Yes --> K["Compute UTC time with Date.UTC and set date time"]
J -- No --> L{"timezoneSign is + or -?"}
L -- No --> Y["Return date"]
L -- Yes --> M["Compute totalOffsetMinutes from timezoneHours and timezoneMinutes"]
M --> N{"timezoneSign is +?"}
N -- Yes --> O["date.setMinutes(date.getMinutes() - totalOffsetMinutes)"]
N -- No --> P["date.setMinutes(date.getMinutes() + totalOffsetMinutes)"]
O --> Y
P --> Y
K --> Y
Y["Return Date object"] --> Q["End"]
Z --> Q
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 and found some issues that need to be addressed.
- In
loadMetadatayou reuse the.bb-view-pdf-dialog-titleclass for both the dialog heading and the metadata value and then select it viaquerySelector, which will only target the first element and may overwrite the title instead of the metadata field; consider using distinct class names or more specific selectors for each role. loadMetadataassumesmetadata,metadata.info, and properties likeCreationDate,Creator,Producer, andPDFFormatVersionalways exist, andgetFilesizeassumesmetadata.contentLengthis defined and small enough to match existing branches; it would be safer to guard these accesses and provide sensible fallbacks when values are missing or unexpected.- There is a lingering
console.log(metadata);and the dropdown toolbar click handler for.dropdown-item-docis not cleaned up indispose, which can lead to noisy logs and potential event handler leaks if the component is initialized multiple times; consider removing the log and unregistering the toolbar handler similarly to the close button.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `loadMetadata` you reuse the `.bb-view-pdf-dialog-title` class for both the dialog heading and the metadata value and then select it via `querySelector`, which will only target the first element and may overwrite the title instead of the metadata field; consider using distinct class names or more specific selectors for each role.
- `loadMetadata` assumes `metadata`, `metadata.info`, and properties like `CreationDate`, `Creator`, `Producer`, and `PDFFormatVersion` always exist, and `getFilesize` assumes `metadata.contentLength` is defined and small enough to match existing branches; it would be safer to guard these accesses and provide sensible fallbacks when values are missing or unexpected.
- There is a lingering `console.log(metadata);` and the dropdown toolbar click handler for `.dropdown-item-doc` is not cleaned up in `dispose`, which can lead to noisy logs and potential event handler leaks if the component is initialized multiple times; consider removing the log and unregistering the toolbar handler similarly to the close button.
## Individual Comments
### Comment 1
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:111-120` </location>
<code_context>
return pdfViewer;
}
+const loadMetadata = (el, pdfViewer, metadata) => {
+ console.log(metadata);
+
+ const filename = el.querySelector('.bb-view-pdf-dialog-filename');
+ const docTitle = el.querySelector('.bb-view-subject');
+ filename.textContent = docTitle.textContent;
+
+ const filesize = el.querySelector('.bb-view-pdf-dialog-filesize');
+ filesize.textContent = getFilesize(metadata);
+
+ const title = el.querySelector('.bb-view-pdf-dialog-title');
+ const author = el.querySelector('.bb-view-pdf-dialog-author');
+ const subject = el.querySelector('.bb-view-pdf-dialog-subject');
+ const keywords = el.querySelector('.bb-view-pdf-dialog-keywords');
+ const created = el.querySelector('.bb-view-pdf-dialog-created');
+ created.textContent = parsePdfDate(metadata.info.CreationDate)?.toLocaleString();
</code_context>
<issue_to_address>
**issue (bug_risk):** Multiple queried metadata elements are never populated, so the dialog will remain empty for those fields.
In `loadMetadata`, `title`, `author`, `subject`, and `keywords` are queried but never assigned from `metadata`, so those dialog rows will always be blank even when the PDF provides values. Please populate them from `metadata` (e.g. `title.textContent = metadata.info.Title || ''`) so all available metadata is shown.
</issue_to_address>
### Comment 2
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:125-126` </location>
<code_context>
+ const author = el.querySelector('.bb-view-pdf-dialog-author');
+ const subject = el.querySelector('.bb-view-pdf-dialog-subject');
+ const keywords = el.querySelector('.bb-view-pdf-dialog-keywords');
+ const created = el.querySelector('.bb-view-pdf-dialog-created');
+ created.textContent = parsePdfDate(metadata.info.CreationDate)?.toLocaleString();
+
+ const modified = el.querySelector('.bb-view-pdf-dialog-modified');
</code_context>
<issue_to_address>
**issue:** Direct access to `metadata.info.*` assumes all properties exist and can throw when metadata is missing.
In `loadMetadata`, please guard these accesses with `metadata && metadata.info` and use optional chaining or fallbacks when assigning to `textContent`, e.g. `created.textContent = metadata.info?.CreationDate ? parsePdfDate(metadata.info.CreationDate)?.toLocaleString() : ''` and similar for the other fields.
</issue_to_address>
### Comment 3
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:128-137` </location>
<code_context>
+ const modified = el.querySelector('.bb-view-pdf-dialog-modified');
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The `modified` and `webview` elements are queried but never used, leaving the dialog fields inconsistent.
`.bb-view-pdf-dialog-modified` and `.bb-view-pdf-dialog-webview` are queried, but `modified.textContent` is never set and `webview` is never used (Razor currently hardcodes `No`). This leaves “Modified” always blank and makes the `webview` query dead code. Either populate `modified` (e.g. from `metadata.info.ModDate` via `parsePdfDate`) and drive `webview` from real document data, or remove these queries to avoid inconsistent UI and unused elements.
Suggested implementation:
```javascript
const created = el.querySelector('.bb-view-pdf-dialog-created');
created.textContent = parsePdfDate(metadata.info.CreationDate)?.toLocaleString();
const modified = el.querySelector('.bb-view-pdf-dialog-modified');
modified.textContent = parsePdfDate(metadata.info.ModDate)?.toLocaleString();
const application = el.querySelector('.bb-view-pdf-dialog-application');
```
1. Locate where `.bb-view-pdf-dialog-webview` is queried in `PdfReader.razor.js` and either:
- Bind it to a real data source (e.g. a boolean or flag in `metadata` that indicates whether the document was viewed via web), or
- Remove the query and any associated unused variable if no such data is available yet.
2. Ensure the Razor markup for the “Modified” field is present and correctly bound to `.bb-view-pdf-dialog-modified` so the populated value is visible.
</issue_to_address>
### Comment 4
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:202-211` </location>
<code_context>
+ return date;
+}
+
+const getFilesize = metadata => {
+ const length = metadata.contentLength;
+ if (length < 1024) {
+ return `${Math.round(length)}B`;
+ }
+ else if (length < 1024 * 1024) {
+ return `${Math.round(length / 1024)}KB`;
+ }
+ else if (length < 1024 * 1024 * 1024) {
+ return `${length / 1024 / 1024}MB`;
+ }
+ else if (length < 1024 * 1024 * 1024 * 1024) {
+ return `${length / 1024 / 1024 / 1024}GB`;
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** `getFilesize` can return `undefined` for large files and uses inconsistent rounding between units.
For `metadata.contentLength >= 1024 * 1024 * 1024 * 1024`, the function returns nothing, so the UI will display `undefined`. Also, MB/GB values are not rounded, unlike B/KB, which may produce long decimals. Please add a final fallback branch and standardize rounding across units (e.g. `return `${(length / 1024 / 1024).toFixed(1)} MB`;`, and similarly for sizes ≥ 1 TB).
</issue_to_address>
### Comment 5
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:446-447` </location>
<code_context>
+ }
+ });
+
+ const closeButton = el.querySelector(".btn-close-doc");
+ EventHandler.on(closeButton, 'click', e => {
+ const dialog = el.querySelector(".bb-view-pdf-info");
+ if (dialog) {
</code_context>
<issue_to_address>
**issue (bug_risk):** `EventHandler.on` is called unconditionally on `closeButton` and can fail if the button is missing.
`dispose` already guards `closeButton` with an `if (closeButton)`, but the event registration path does not and will throw if the button is absent (e.g., due to markup or conditional rendering changes). Wrapping the `EventHandler.on(closeButton, 'click', ...)` call in the same null check would avoid this and keep the behavior consistent.
</issue_to_address>
### Comment 6
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor:104-113` </location>
<code_context>
+ <div class="bb-view-pdf-info">
+ <div class="bb-view-pdf-backdrop"></div>
+ <div class="bb-view-pdf-dialog">
+ <div class="bb-view-pdf-dialog-title mb-3">Document properties</div>
+ <div class="bb-view-pdf-dialog-item">
+ <div class="bb-view-pdf-dialog-label">File name:</div>
+ <div class="bb-view-pdf-dialog-filename"></div>
+ </div>
+ <div class="bb-view-pdf-dialog-item">
+ <div class="bb-view-pdf-dialog-label">File size:</div>
+ <div class="bb-view-pdf-dialog-filesize"></div>
+ </div>
+ <div class="divider">
+ <div class="divider-mask"></div>
+ </div>
+ <div class="bb-view-pdf-dialog-item">
+ <div class="bb-view-pdf-dialog-label">Title:</div>
+ <div class="bb-view-pdf-dialog-title"></div>
+ </div>
+ <div class="bb-view-pdf-dialog-item">
</code_context>
<issue_to_address>
**issue (bug_risk):** The same CSS class is used for the dialog header and the PDF title field, which breaks the JS metadata binding.
`loadMetadata` uses `el.querySelector('.bb-view-pdf-dialog-title')` and assumes it points to the Title metadata field, but in the markup that class is applied to both the header and the title value row. Because `querySelector` returns the first match (the header), the title cell never gets populated. Please give the title value a unique class (e.g. `.bb-view-pdf-dialog-title-value`) and update the JS selector to match.
</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
+111
to
+120
| const loadMetadata = (el, pdfViewer, metadata) => { | ||
| console.log(metadata); | ||
|
|
||
| const filename = el.querySelector('.bb-view-pdf-dialog-filename'); | ||
| const docTitle = el.querySelector('.bb-view-subject'); | ||
| filename.textContent = docTitle.textContent; | ||
|
|
||
| const filesize = el.querySelector('.bb-view-pdf-dialog-filesize'); | ||
| filesize.textContent = getFilesize(metadata); | ||
|
|
There was a problem hiding this comment.
issue (bug_risk): Multiple queried metadata elements are never populated, so the dialog will remain empty for those fields.
In loadMetadata, title, author, subject, and keywords are queried but never assigned from metadata, so those dialog rows will always be blank even when the PDF provides values. Please populate them from metadata (e.g. title.textContent = metadata.info.Title || '') so all available metadata is shown.
Comment on lines
+125
to
+126
| const created = el.querySelector('.bb-view-pdf-dialog-created'); | ||
| created.textContent = parsePdfDate(metadata.info.CreationDate)?.toLocaleString(); |
There was a problem hiding this comment.
issue: Direct access to metadata.info.* assumes all properties exist and can throw when metadata is missing.
In loadMetadata, please guard these accesses with metadata && metadata.info and use optional chaining or fallbacks when assigning to textContent, e.g. created.textContent = metadata.info?.CreationDate ? parsePdfDate(metadata.info.CreationDate)?.toLocaleString() : '' and similar for the other fields.
Comment on lines
+128
to
+137
| const modified = el.querySelector('.bb-view-pdf-dialog-modified'); | ||
|
|
||
| const application = el.querySelector('.bb-view-pdf-dialog-application'); | ||
| application.textContent = metadata.info.Creator; | ||
|
|
||
| const producer = el.querySelector('.bb-view-pdf-dialog-producer'); | ||
| producer.textContent = metadata.info.Producer; | ||
|
|
||
| const version = el.querySelector('.bb-view-pdf-dialog-version'); | ||
| version.textContent = metadata.info.PDFFormatVersion; |
There was a problem hiding this comment.
suggestion (bug_risk): The modified and webview elements are queried but never used, leaving the dialog fields inconsistent.
.bb-view-pdf-dialog-modified and .bb-view-pdf-dialog-webview are queried, but modified.textContent is never set and webview is never used (Razor currently hardcodes No). This leaves “Modified” always blank and makes the webview query dead code. Either populate modified (e.g. from metadata.info.ModDate via parsePdfDate) and drive webview from real document data, or remove these queries to avoid inconsistent UI and unused elements.
Suggested implementation:
const created = el.querySelector('.bb-view-pdf-dialog-created');
created.textContent = parsePdfDate(metadata.info.CreationDate)?.toLocaleString();
const modified = el.querySelector('.bb-view-pdf-dialog-modified');
modified.textContent = parsePdfDate(metadata.info.ModDate)?.toLocaleString();
const application = el.querySelector('.bb-view-pdf-dialog-application');- Locate where
.bb-view-pdf-dialog-webviewis queried inPdfReader.razor.jsand either:- Bind it to a real data source (e.g. a boolean or flag in
metadatathat indicates whether the document was viewed via web), or - Remove the query and any associated unused variable if no such data is available yet.
- Bind it to a real data source (e.g. a boolean or flag in
- Ensure the Razor markup for the “Modified” field is present and correctly bound to
.bb-view-pdf-dialog-modifiedso the populated value is visible.
Comment on lines
+202
to
+211
| const getFilesize = metadata => { | ||
| const length = metadata.contentLength; | ||
| if (length < 1024) { | ||
| return `${Math.round(length)}B`; | ||
| } | ||
| else if (length < 1024 * 1024) { | ||
| return `${Math.round(length / 1024)}KB`; | ||
| } | ||
| else if (length < 1024 * 1024 * 1024) { | ||
| return `${length / 1024 / 1024}MB`; |
There was a problem hiding this comment.
issue (bug_risk): getFilesize can return undefined for large files and uses inconsistent rounding between units.
For metadata.contentLength >= 1024 * 1024 * 1024 * 1024, the function returns nothing, so the UI will display undefined. Also, MB/GB values are not rounded, unlike B/KB, which may produce long decimals. Please add a final fallback branch and standardize rounding across units (e.g. return ${(length / 1024 / 1024).toFixed(1)} MB;, and similarly for sizes ≥ 1 TB).
Comment on lines
+104
to
+113
| <div class="bb-view-pdf-dialog-title mb-3">Document properties</div> | ||
| <div class="bb-view-pdf-dialog-item"> | ||
| <div class="bb-view-pdf-dialog-label">File name:</div> | ||
| <div class="bb-view-pdf-dialog-filename"></div> | ||
| </div> | ||
| <div class="bb-view-pdf-dialog-item"> | ||
| <div class="bb-view-pdf-dialog-label">File size:</div> | ||
| <div class="bb-view-pdf-dialog-filesize"></div> | ||
| </div> | ||
| <div class="divider"> |
There was a problem hiding this comment.
issue (bug_risk): The same CSS class is used for the dialog header and the PDF title field, which breaks the JS metadata binding.
loadMetadata uses el.querySelector('.bb-view-pdf-dialog-title') and assumes it points to the Title metadata field, but in the markup that class is applied to both the header and the title value row. Because querySelector returns the first match (the header), the title cell never gets populated. Please give the title value a unique class (e.g. .bb-view-pdf-dialog-title-value) and update the JS selector to match.
There was a problem hiding this comment.
Pull request overview
This PR adds document properties functionality to the PdfReader component, allowing users to view metadata information about loaded PDF files. The implementation includes a modal dialog that displays file information, document metadata, and PDF-specific properties.
Key Changes:
- Added a document properties dialog with metadata display functionality including file name, size, title, author, creation date, PDF version, and page information
- Implemented PDF date parsing and file size formatting utilities
- Fixed several code quality issues including Math.round parameter usage and classList assignment
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| PdfReader.razor.js | Added loadMetadata function, parsePdfDate utility, getFilesize formatter, and event handlers for the document properties dialog; fixed worker path and code quality issues |
| PdfReader.razor | Added HTML structure for the document properties modal dialog with metadata fields and close button |
| PdfReader.razor.css | Added styling for the document properties dialog, backdrop, and layout |
| en.json | Added English localization for "CloseButtonText" |
| zh.json | Added Chinese localization for "CloseButtonText" |
| BootstrapBlazor.PdfReader.csproj | Bumped version from 10.0.6 to 10.0.7 |
Comments suppressed due to low confidence (10)
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:121
- Unused variable title.
const title = el.querySelector('.bb-view-pdf-dialog-title');
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:122
- Unused variable author.
const author = el.querySelector('.bb-view-pdf-dialog-author');
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:123
- Unused variable subject.
const subject = el.querySelector('.bb-view-pdf-dialog-subject');
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:124
- Unused variable keywords.
const keywords = el.querySelector('.bb-view-pdf-dialog-keywords');
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:128
- Unused variable modified.
const modified = el.querySelector('.bb-view-pdf-dialog-modified');
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:148
- Unused variable webview.
const webview = el.querySelector('.bb-view-pdf-dialog-webview');
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:456
- Unused variable scaleEl.
const scaleEl = el.querySelector(".bb-view-scale-input");
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:557
- The initial value of v is unused, since it is always overwritten.
let v = 100;
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:418
- Avoid automated semicolon insertion (94% of all statements in the enclosing function have an explicit semicolon).
})
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:480
- Avoid automated semicolon insertion (91% of all statements in the enclosing function have an explicit semicolon).
delete el.widths
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| producer.textContent = metadata.info.Producer; | ||
|
|
||
| const version = el.querySelector('.bb-view-pdf-dialog-version'); | ||
| version.textContent = metadata.info.PDFFormatVersion; |
There was a problem hiding this comment.
Potential null reference error if metadata.info.PDFFormatVersion is undefined. Add a null check or use optional chaining: version.textContent = metadata.info.PDFFormatVersion || ''
Suggested change
| version.textContent = metadata.info.PDFFormatVersion; | |
| version.textContent = metadata.info?.PDFFormatVersion || ''; |
| return `${Math.round(length / 1024)}KB`; | ||
| } | ||
| else if (length < 1024 * 1024 * 1024) { | ||
| return `${length / 1024 / 1024}MB`; |
There was a problem hiding this comment.
The MB calculation should be rounded for consistency with KB formatting. Consider: return \${Math.round(length / 1024 / 1024)}MB``
Suggested change
| return `${length / 1024 / 1024}MB`; | |
| return `${Math.round(length / 1024 / 1024)}MB`; |
Comment on lines
+211
to
+214
| return `${length / 1024 / 1024}MB`; | ||
| } | ||
| else if (length < 1024 * 1024 * 1024 * 1024) { | ||
| return `${length / 1024 / 1024 / 1024}GB`; |
There was a problem hiding this comment.
The GB calculation should be rounded for consistency with KB and MB formatting. Consider: return \${Math.round(length / 1024 / 1024 / 1024)}GB``
Suggested change
| return `${length / 1024 / 1024}MB`; | |
| } | |
| else if (length < 1024 * 1024 * 1024 * 1024) { | |
| return `${length / 1024 / 1024 / 1024}GB`; | |
| return `${Math.round(length / 1024 / 1024)}MB`; | |
| } | |
| else if (length < 1024 * 1024 * 1024 * 1024) { | |
| return `${Math.round(length / 1024 / 1024 / 1024)}GB`; |
Comment on lines
+211
to
+215
| return `${length / 1024 / 1024}MB`; | ||
| } | ||
| else if (length < 1024 * 1024 * 1024 * 1024) { | ||
| return `${length / 1024 / 1024 / 1024}GB`; | ||
| } |
There was a problem hiding this comment.
The function doesn't have an explicit return statement for the GB case and beyond. Add an explicit return statement to ensure the function always returns a value, or add an else clause with a fallback.
Suggested change
| return `${length / 1024 / 1024}MB`; | |
| } | |
| else if (length < 1024 * 1024 * 1024 * 1024) { | |
| return `${length / 1024 / 1024 / 1024}GB`; | |
| } | |
| return `${Math.round(length / 1024 / 1024)}MB`; | |
| } | |
| else if (length < 1024 * 1024 * 1024 * 1024) { | |
| return `${Math.round(length / 1024 / 1024 / 1024)}GB`; | |
| } | |
| else { | |
| return `${Math.round(length / 1024 / 1024 / 1024 / 1024)}TB`; | |
| } |
Comment on lines
+104
to
+118
| <div class="bb-view-pdf-dialog-title mb-3">Document properties</div> | ||
| <div class="bb-view-pdf-dialog-item"> | ||
| <div class="bb-view-pdf-dialog-label">File name:</div> | ||
| <div class="bb-view-pdf-dialog-filename"></div> | ||
| </div> | ||
| <div class="bb-view-pdf-dialog-item"> | ||
| <div class="bb-view-pdf-dialog-label">File size:</div> | ||
| <div class="bb-view-pdf-dialog-filesize"></div> | ||
| </div> | ||
| <div class="divider"> | ||
| <div class="divider-mask"></div> | ||
| </div> | ||
| <div class="bb-view-pdf-dialog-item"> | ||
| <div class="bb-view-pdf-dialog-label">Title:</div> | ||
| <div class="bb-view-pdf-dialog-title"></div> |
There was a problem hiding this comment.
The class name bb-view-pdf-dialog-title is used for both the dialog header (line 104) and a data field for the document title (line 118). This creates ambiguity and makes it impossible to target these elements separately. Consider renaming one of them, e.g., use bb-view-pdf-dialog-header for the dialog header at line 104.
| size.textContent = `${(viewport.width / 72).toFixed(2)} * ${(viewport.height / 72).toFixed(2)} in (portrait)`; | ||
| }); | ||
|
|
||
| const webview = el.querySelector('.bb-view-pdf-dialog-webview'); |
There was a problem hiding this comment.
Variable webview is declared but never used. It should be populated with fast web view information or removed if not needed.
Suggested change
| const webview = el.querySelector('.bb-view-pdf-dialog-webview'); |
| const subject = el.querySelector('.bb-view-pdf-dialog-subject'); | ||
| const keywords = el.querySelector('.bb-view-pdf-dialog-keywords'); | ||
| const created = el.querySelector('.bb-view-pdf-dialog-created'); | ||
| created.textContent = parsePdfDate(metadata.info.CreationDate)?.toLocaleString(); |
There was a problem hiding this comment.
The code doesn't handle cases where metadata.info.CreationDate might be undefined or null. Consider adding a null check or providing a fallback value to avoid displaying "undefined" or causing an error: created.textContent = parsePdfDate(metadata.info.CreationDate)?.toLocaleString() || 'N/A'
Suggested change
| created.textContent = parsePdfDate(metadata.info.CreationDate)?.toLocaleString(); | |
| created.textContent = parsePdfDate(metadata.info.CreationDate)?.toLocaleString() || 'N/A'; |
| const modified = el.querySelector('.bb-view-pdf-dialog-modified'); | ||
|
|
||
| const application = el.querySelector('.bb-view-pdf-dialog-application'); | ||
| application.textContent = metadata.info.Creator; |
There was a problem hiding this comment.
Potential null reference error if metadata.info.Creator is undefined. Add a null check or use optional chaining: application.textContent = metadata.info.Creator || ''
Suggested change
| application.textContent = metadata.info.Creator; | |
| application.textContent = metadata.info?.Creator || ''; |
| application.textContent = metadata.info.Creator; | ||
|
|
||
| const producer = el.querySelector('.bb-view-pdf-dialog-producer'); | ||
| producer.textContent = metadata.info.Producer; |
There was a problem hiding this comment.
Potential null reference error if metadata.info.Producer is undefined. Add a null check or use optional chaining: producer.textContent = metadata.info.Producer || ''
Suggested change
| producer.textContent = metadata.info.Producer; | |
| producer.textContent = metadata.info?.Producer || ''; |
| } | ||
|
|
||
| const loadMetadata = (el, pdfViewer, metadata) => { | ||
| console.log(metadata); |
There was a problem hiding this comment.
Remove the console.log statement before merging to production. Debug logging should not be left in production code.
Suggested change
| console.log(metadata); |
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 #750
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Add a document properties dialog to the PDF reader and wire it to the toolbar while aligning worker configuration and minor UI behavior.
New Features:
Enhancements: