215 embedpdf for main viewer#228
Conversation
…inityBowman/corates into 215-embedpdf-for-main-viewer
…inityBowman/corates into 215-embedpdf-for-main-viewer
…inityBowman/corates into 215-embedpdf-for-main-viewer
…inityBowman/corates into 215-embedpdf-for-main-viewer
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis pull request migrates the PDF viewer implementation from PDF.js to EmbedPDF with a new Preact-based UI system, reorganizes PDF components into a new directory structure, updates module path aliases, and removes the legacy SolidJS-based PDF viewer implementation including associated utilities and configuration. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
corates-workers-prod | 576a135 | Jan 05 2026, 06:31 PM |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Fix all issues with AI Agents 🤖
In
@packages/web/src/components/pdf/embedpdf/preact/src/components/capture-dialog.tsx:
- Around line 68-74: handleImageLoad revokes urlRef.current as soon as the image
loads which can break the <img> if the component re-renders because previewUrl
still points to the revoked object URL; remove the premature cleanup in
handleImageLoad (or delete handleImageLoad entirely) and instead perform URL
cleanup only in handleClose: revoke urlRef.current, set urlRef.current = null,
and clear previewUrl state there so the object URL remains valid for the
lifetime of the dialog; ensure you update any references to handleImageLoad so
the img onLoad no longer revokes the URL.
In
@packages/web/src/components/pdf/embedpdf/preact/src/components/document-menu.tsx:
- Around line 47-50: handleFullscreen builds a CSS selector with
`#${documentId}` which can break if documentId contains characters invalid in
selectors; update handleFullscreen (and the call to
fullscreenProvider?.toggleFullscreen) to sanitize the id with
CSS.escape(documentId) (or validate that documentId is a valid CSS identifier)
before constructing the selector, then call
fullscreenProvider?.toggleFullscreen(`#${escapedId}`) and finally
setIsMenuOpen(false).
In
@packages/web/src/components/pdf/embedpdf/preact/src/components/document-password-prompt.tsx:
- Around line 71-77: Replace the unicode "✕" in the close button with the
project's SVG CloseIcon component: import CloseIcon from the icons module,
render <CloseIcon /> (or the project's equivalent) inside the button used for
provides?.closeDocument(documentState.id), keep the existing onClick and
disabled={isRetrying} behavior, and add an accessible label (e.g.,
aria-label="Close") or visually-hidden text so screen readers know the button
purpose.
In
@packages/web/src/components/pdf/embedpdf/preact/src/components/empty-state.tsx:
- Around line 3-5: Rename the parameter in the EmptyStateProps callback from
_documentId to documentId to reflect that the value is used; update the
interface declaration for onDocumentOpened (and any matching implementations or
usages of EmptyStateProps/onDocumentOpened) to accept documentId: string instead
of _documentId so the name is no longer misleading.
In
@packages/web/src/components/pdf/embedpdf/preact/src/components/loading-spinner.tsx:
- Around line 13-34: The LoadingSpinner component is missing ARIA attributes and
a screen-reader fallback; update the LoadingSpinner function to add
role="status", aria-live="polite" and aria-busy="true" on the container, mark
the decorative SVG as aria-hidden="true", and render a visually-hidden text node
(e.g., "Loading...") when the message prop is not provided so screen readers
always get context; keep the visible message rendering unchanged and use the
existing sizeClasses/props to locate the component.
In
@packages/web/src/components/pdf/embedpdf/preact/src/components/page-controls.tsx:
- Around line 124-126: The page number input and the totalPages span are
rendered adjacent with no separator; update the JSX in the page controls
component (the form containing the page input and the span that references
totalPages) to insert an accessible separator (for example a small span
containing " / " or " of " with appropriate spacing and aria-hidden if purely
visual) between the input element and the {totalPages} span so the UI reads like
"3 / 15" instead of "3 15".
In
@packages/web/src/components/pdf/embedpdf/preact/src/components/pdf-picker.tsx:
- Around line 11-27: The useMemo hook (creating sortedPdfs) is called after an
early return in PdfPicker which violates the Rules of Hooks; move the useMemo
(and any other hooks) above the conditional return so that all hooks (e.g., the
existing useState isMenuOpen and the useMemo that defines sortedPdfs) are
invoked unconditionally at the top of PdfPicker, then perform the "if (!pdfs ||
pdfs.length <= 1) return null" check and use the already-computed sortedPdfs
below.
In
@packages/web/src/components/pdf/embedpdf/preact/src/components/selection-selection-menu.tsx:
- Around line 19-36: The setTimeout in handleCopy can update state after
unmount; modify handleCopy to store the timeout id (e.g., timerRef.current =
window.setTimeout(...)) instead of calling setTimeout directly, and add a
useEffect cleanup that clears the timeout (clearTimeout(timerRef.current)) or
use a mountedRef to guard setCopied so no state is set when unmounted; keep the
existing scope.copyToClipboard() and scope.clear() calls but ensure the timeout
is cancelled on unmount to prevent setCopied from running later.
In
@packages/web/src/components/pdf/embedpdf/preact/src/components/tab-context-menu.tsx:
- Line 76: The label "View {index + 2} ({view.documentIds.length} docs)" is
unclear because it assumes the current view is "View 1"; either add a short
comment above this line explaining that naming starts at 2 due to the current
view occupying slot 1, or replace the computed label with an explicit identifier
(e.g. use view.name || `View ${view.id || index + 1}`) so naming is based on the
view's stable id/name rather than index arithmetic; update the string generation
point in tab-context-menu.tsx where index and view.documentIds are used to
implement the chosen approach.
- Around line 22-30: The effect only handles outside clicks; add an Escape key
handler inside the same useEffect: register a 'keydown' listener that checks for
e.key === 'Escape' (or e.key === 'Esc' for legacy) and calls onClose(), and
remove that listener in the cleanup. Update the effect (which currently defines
handleClickOutside and uses menuRef, onClose) to also define handleKeyDown and
attach document.addEventListener('keydown', handleKeyDown) so the context menu
closes on Escape and ensure both listeners are removed in the returned cleanup.
In
@packages/web/src/components/pdf/embedpdf/preact/src/components/thumbnails-sidebar.tsx:
- Around line 12-76: Thumbnail items lack keyboard and ARIA support; update the
clickable thumbnail container rendered inside ThumbnailsPane (the element using
key={m.pageIndex} and onClick calling provides?.scrollToPage) to be
keyboard-focusable and accessible by adding tabIndex=0, role="button" and
aria-pressed or aria-current (based on state.currentPage === m.pageIndex + 1),
and handle Enter/Space key events to invoke provides.scrollToPage({ pageNumber:
m.pageIndex + 1 }); also ensure ThumbImg has alt text or aria-hidden as
appropriate and that the page number span exposes its label (e.g.,
aria-label={`Page ${m.pageIndex + 1}`}), so screen readers and keyboard users
can interact with thumbnails.
- Around line 4-9: ThumbnailsSidebarProps declares an onClose callback but the
component destructures it as _onClose and never uses it; either remove onClose
from ThumbnailsSidebarProps and from the ThumbnailsSidebar parameter list, or
restore and use the prop: change the destructuring to { documentId, onClose } in
ThumbnailsSidebar, wire onClose to the component's close control (e.g., call
onClose() when a close button or escape handler runs), and keep the prop
optional in the type; update any JSX that expected the prop accordingly.
In
@packages/web/src/components/pdf/embedpdf/preact/src/components/viewer-toolbar.tsx:
- Around line 89-98: The commented-out Redact button block should either be
removed or documented; locate the JSX block that calls onClick={() =>
onModeChange('redact')} and references mode === 'redact' (the commented Redact
button) and either delete that entire commented fragment or replace it with a
brief inline comment explaining why the button is disabled/hidden (e.g., feature
incomplete, gated behind flag) and reference that the RedactionToolbar
conditional render remains (RedactionToolbar) so future devs understand the
partial implementation.
In @packages/web/src/components/pdf/embedpdf/preact/src/config/commands.ts:
- Around line 339-341: The active checker may throw if the 'interaction-manager'
plugin is absent; update the expression in the active property to use optional
chaining for the plugin and documents lookup (e.g., change
state.plugins['interaction-manager'].documents[documentId]?.activeMode ===
'marqueeCapture' to
state.plugins['interaction-manager']?.documents?.[documentId]?.activeMode ===
'marqueeCapture') so it safely returns false when the plugin or documents is
undefined.
In @packages/web/src/components/pdf/embedpdf/preact/src/config/translations.ts:
- Around line 3-4: The import for SEARCH_PLUGIN_ID uses the React entry point
but this file is Preact-based; update the import from
'@embedpdf/plugin-search/react' to the Preact entry
'@embedpdf/plugin-search/preact' so it matches ZOOM_PLUGIN_ID's Preact usage
(leave ZOOM_PLUGIN_ID import as is), then run type checks/build to ensure no
other React-specific symbols are referenced.
In @packages/web/src/components/pdf/embedpdf/preact/src/viewer.tsx:
- Around line 61-64: The local SidebarState type declared in viewer.tsx
duplicates the existing SidebarState type defined in config types; remove the
local type declaration and import the SidebarState type from the shared types
module, then ensure all references in viewer.tsx use the imported SidebarState
(update any local usages or props/interfaces that referenced the removed
declaration).
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/web/src/components/checklist/ChecklistWithPdf.jsx (1)
13-27: Refactor to reduce prop drilling—component receives >10 props.This component receives more than 10 props, which violates the coding guideline that SolidJS components should receive at most 1-5 props (local config only). Many of these props appear to be application state that could be sourced from stores instead of prop-drilled.
Consider refactoring to import stores directly for shared state like PDF data, selected PDF ID, and read-only status. Keep only local configuration props like
checklistTypeand callbacks.As per coding guidelines: "SolidJS components should receive at most 1-5 props (local config only, not shared state)" and "Do NOT prop-drill application state in SolidJS - import stores directly where needed."
🧹 Nitpick comments (24)
packages/web/src/components/pdf/embedpdf/preact/src/components/empty-state.tsx (2)
10-20: Consider user feedback for error cases.Two UX concerns:
- If
providesis undefined, clicking the button silently does nothing.- On failure, only console logging occurs—users receive no feedback.
Consider disabling the button when the capability is unavailable and showing a toast or inline error message on failure.
Suggested approach
export function EmptyState({ onDocumentOpened }: EmptyStateProps) { const { provides } = useDocumentManagerCapability(); + const isCapabilityAvailable = !!provides?.openFileDialog; const handleOpenFile = () => { const openTask = provides?.openFileDialog(); openTask?.wait( result => { onDocumentOpened?.(result.documentId); }, error => { console.error('Open file failed:', error); + // TODO: Show user-facing error notification }, ); };Then in the button:
<button onClick={handleOpenFile} disabled={!isCapabilityAvailable} className='... disabled:opacity-50 disabled:cursor-not-allowed' >
52-60: Add explicittype="button"attribute.Buttons default to
type="submit"inside forms. Adding explicittype="button"prevents accidental form submissions if this component is ever rendered within a form context.<button + type='button' onClick={handleOpenFile} className='inline-flex items-center gap-2 rounded-lg bg-indigo-600 px-6 py-3 font-semibold text-white shadow-lg transition-all hover:bg-indigo-700 hover:shadow-xl' >packages/web/src/components/pdf/embedpdf/preact/src/components/page-settings-menu.tsx (1)
60-91: Consider distinct icons for Odd vs. Even page layouts.The implementation correctly manages spread modes with proper state highlighting. However, both "Odd Pages" (Line 76) and "Even Pages" (Line 86) use the same
BookOpenIcon, which could make it harder for users to quickly distinguish between these layout options visually.If differentiated icons are available, consider using them to improve visual clarity. If not, the current implementation is acceptable.
packages/web/src/components/pdf/embedpdf/preact/src/components/loading-spinner.tsx (1)
15-15: Consider making the color configurable.The
text-gray-600color is hardcoded, which may not align with different themes or contexts where this component is used. Consider accepting a color prop or using a CSS variable for better flexibility.packages/web/src/components/pdf/embedpdf/preact/src/components/search-sidebar.tsx (3)
77-87: Consider usinge.currentTargetto avoid type casting.Since the event is already typed as
TargetedEvent<HTMLInputElement>, you can usee.currentTarget.valuedirectly instead of castinge.target.Proposed refactor
const handleInputChange = (e: TargetedEvent<HTMLInputElement>) => { - const value = (e.target as HTMLInputElement).value; + const value = e.currentTarget.value; setInputValue(value); // Trigger search immediately on user input if (value === '') { provides?.stopSearch(); } else { provides?.searchAllPages(value); } };
117-121: Remove or document the commented-out code.The commented-out
center: trueoption should either be removed if it's not needed, or the comment should explain why centering is disabled.
177-200: Consider usinge.currentTargetfor checkbox handlers.Similar to the input handler, the checkbox event handlers can use
e.currentTarget.checkedinstead of type casting.Proposed refactor
<label className='flex items-center text-sm text-gray-700'> <input type='checkbox' checked={state.flags.includes(MatchFlag.MatchCase)} onChange={e => - handleFlagChange(MatchFlag.MatchCase, (e.target as HTMLInputElement).checked) + handleFlagChange(MatchFlag.MatchCase, e.currentTarget.checked) } className='mr-2 h-4 w-4 rounded border-gray-300 text-blue-600 focus:ring-blue-500' /> {translate('search.caseSensitive')} </label> <label className='flex items-center text-sm text-gray-700'> <input type='checkbox' checked={state.flags.includes(MatchFlag.MatchWholeWord)} onChange={e => - handleFlagChange(MatchFlag.MatchWholeWord, (e.target as HTMLInputElement).checked) + handleFlagChange(MatchFlag.MatchWholeWord, e.currentTarget.checked) } className='mr-2 h-4 w-4 rounded border-gray-300 text-blue-600 focus:ring-blue-500' /> {translate('search.wholeWord')} </label>packages/web/vite.config.js (1)
33-44: Consider consolidating or clarifying preact directory structure.The plugin configuration correctly isolates SolidJS and Preact to specific directories. However, the presence of both
preactandpreact-2directories suggests unclear organization. Consider:
- Using more descriptive names if they serve different purposes (e.g.,
preact/pdf,preact/legacy)- Consolidating into a single
preactdirectory if the split is temporary- Adding a comment explaining why two separate directories are needed
This would improve maintainability and reduce confusion for future developers.
packages/web/src/components/pdf/embedpdf/preact/src/components/thumbnails-sidebar.tsx (2)
14-14: Remove narrating comment.This comment simply states what the code does rather than explaining why. Per coding guidelines, comments should explain intent, context, or rationale, not narrate the obvious.
Proposed fix
- {/* Thumbnails */} <div className='flex-1 overflow-hidden'>
13-75: Prefer Tailwind CSS classes over inline styles for consistency.The component mixes Tailwind CSS classes with extensive inline styles. For maintainability and consistency with the coding guidelines, prefer Tailwind utilities throughout.
Example conversion to Tailwind
Instead of inline styles like:
style={{ position: 'absolute', width: '100%', cursor: 'pointer', padding: '8px', }}Use Tailwind classes:
className='absolute w-full cursor-pointer p-2'For dynamic styles (borders, shadows based on current page), consider using Tailwind's conditional classes or CSS custom properties defined in your theme. The hardcoded colors
#3b82f6and#d1d5dbappear to be Tailwind'sblue-500andgray-300respectively.packages/web/src/components/pdf/embedpdf/preact/src/components/ui/dialog-footer.tsx (1)
8-13: Consider using tailwind-merge for className composition.The package.json shows
tailwind-mergewas added as a dependency. For more robust class merging that handles Tailwind CSS conflicts properly, consider using it here instead of string concatenation.Proposed improvement using tailwind-merge
+import { twMerge } from 'tailwind-merge'; import { ReactNode } from 'react'; type DialogFooterProps = { children: ReactNode; className?: string; }; export function DialogFooter({ children, className = '' }: DialogFooterProps) { return ( - <div className={`flex justify-end gap-3 border-t border-gray-200 pt-4 ${className}`}> + <div className={twMerge('flex justify-end gap-3 border-t border-gray-200 pt-4', className)}> {children} </div> ); }packages/web/src/components/pdf/embedpdf/preact/src/components/print-dialog.tsx (2)
43-49: Consider handling edge cases forscrollState.currentPage.If
scrollState.currentPageis undefined or 0 (e.g., before the document fully loads), the "current page" option might produce an invalid or unexpected page range. Consider adding a fallback or disabling the "current" option when the page isn't yet determined.Suggested defensive handling
if (selection === 'current') { - pageRange = String(scrollState.currentPage); + pageRange = String(scrollState.currentPage || 1); } else if (selection === 'custom') {
122-137: No validation of custom page range format.The custom page range input accepts any string without validating the format (e.g., "1-3, 5, 8-10"). Invalid input will be passed directly to the print API, which may fail silently or produce unexpected behavior. Consider adding client-side validation or at minimum displaying an error if the print fails due to invalid range.
packages/web/src/components/pdf/embedpdf/preact/src/components/icons/index.tsx (1)
779-792: Some icons have hardcoded#000000strokes instead ofcurrentColor.Several icons (PenIcon, HighlightIcon, SquigglyIcon, StrikethroughIcon, UnderlineIcon) use hardcoded
stroke='#000000'for internal paths, which prevents theming via CSScolor. This may cause visibility issues in dark mode or themed contexts.Example fix for PenIcon
<g transform='rotate(47.565 12.1875 10.75)'> <path - stroke='#000000' + stroke='currentColor' d='m14.18752,16.75l0,-12c0,-1.1 -0.9,-2 -2,-2s-2,0.9 -2,2l0,12l2,2l2,-2z' /> - <path stroke='#000000' d='m10.18752,6.75l4,0' /> + <path stroke='currentColor' d='m10.18752,6.75l4,0' /> </g>Also applies to: 882-888, 946-956, 974-979, 996-1002
packages/web/src/components/pdf/embedpdf/preact/src/components/ui/toolbar-divider.tsx (1)
6-12: Consider usingtwMergefor className merging.The component works correctly, but the simple string concatenation in lines 9-10 could lead to Tailwind class conflicts if
classNameprop contains overlapping utilities. TheToolbarButtoncomponent in this same UI package usestwMergefor proper class merging.Optional refactor for consistent class merging
+import { twMerge } from 'tailwind-merge'; + type ToolbarDividerProps = { orientation?: 'vertical' | 'horizontal'; className?: string; }; export function ToolbarDivider({ orientation = 'vertical', className = '' }: ToolbarDividerProps) { const dividerClasses = - orientation === 'horizontal' ? - `my-1 h-px w-full bg-gray-300 ${className}` - : `mx-1 h-6 w-px bg-gray-300 ${className}`; + twMerge( + 'bg-gray-300', + orientation === 'horizontal' ? 'my-1 h-px w-full' : 'mx-1 h-6 w-px', + className + ); return <div className={dividerClasses} />; }This matches the pattern used in
ToolbarButtoncomponent and prevents Tailwind class conflicts.packages/web/src/components/pdf/embedpdf/preact/src/components/ui/dialog-content.tsx (1)
8-10: Consider using twMerge for consistent class merging.The component uses template literal concatenation for className, while other UI components in this directory (like toolbar-button.tsx) use
twMergefrom tailwind-merge. Using twMerge would prevent duplicate classes and ensure consistent class resolution behavior across the UI component library.Proposed refactor using twMerge
import { ReactNode } from 'react'; +import { twMerge } from 'tailwind-merge'; type DialogContentProps = { children: ReactNode; className?: string; }; export function DialogContent({ children, className = '' }: DialogContentProps) { - return <div className={`space-y-6 ${className}`}>{children}</div>; + return <div className={twMerge('space-y-6', className)}>{children}</div>; }packages/web/src/components/pdf/embedpdf/preact/src/components/redaction-toolbar.tsx (1)
55-73: Consider using ToolbarButton for action buttons to maintain consistency.The toggle buttons use
ToolbarButtonfrom the UI primitives, but the commit and clear action buttons use raw<button>elements with inline styles. Consider usingToolbarButtonwith appropriate props for visual and behavioral consistency across the toolbar.Proposed refactor for consistency
- <button - onClick={handleCommitPending} - disabled={state.pendingCount === 0} - className='rounded p-2 text-green-600 transition-colors hover:bg-green-50 disabled:cursor-not-allowed disabled:text-gray-400 disabled:hover:bg-transparent' - aria-label='Apply redactions' - title={`Apply ${state.pendingCount} pending redaction(s)`} - > - <CheckIcon className='h-4 w-4' /> - </button> - - <button - onClick={handleClearPending} - disabled={state.pendingCount === 0} - className='rounded p-2 text-red-600 transition-colors hover:bg-red-50 disabled:cursor-not-allowed disabled:text-gray-400 disabled:hover:bg-transparent' - aria-label='Clear redactions' - title={`Clear ${state.pendingCount} pending redaction(s)`} - > - <CloseIcon className='h-4 w-4' /> - </button> + <ToolbarButton + onClick={handleCommitPending} + disabled={state.pendingCount === 0} + aria-label='Apply redactions' + title={`Apply ${state.pendingCount} pending redaction(s)`} + className='text-green-600 hover:bg-green-50' + > + <CheckIcon className='h-4 w-4' /> + </ToolbarButton> + + <ToolbarButton + onClick={handleClearPending} + disabled={state.pendingCount === 0} + aria-label='Clear redactions' + title={`Clear ${state.pendingCount} pending redaction(s)`} + className='text-red-600 hover:bg-red-50' + > + <CloseIcon className='h-4 w-4' /> + </ToolbarButton>packages/web/src/components/pdf/embedpdf/preact/src/components/annotation-toolbar.tsx (1)
30-40: Consider adding proper typing instead ofas anycast.The
defaults as anycast bypasses type safety. If theAnnotationTooltype doesn't include the expected color properties, consider extending the type or using a type guard.Potential improvement
function extractToolColors(tools: AnnotationTool[]): ToolColors { const colors: ToolColors = {}; tools.forEach(tool => { - const defaults = tool.defaults as any; + const defaults = tool.defaults as Record<string, unknown>; colors[tool.id] = { - primaryColor: defaults.strokeColor || defaults.color || defaults.fontColor, - secondaryColor: defaults.color, + primaryColor: (defaults.strokeColor ?? defaults.color ?? defaults.fontColor) as string | undefined, + secondaryColor: defaults.color as string | undefined, }; }); return colors; }packages/web/src/components/pdf/embedpdf/preact/src/components/ui/button.tsx (1)
36-40: Consider usingtwMergefor cleaner class composition.The className string with multiple ternary operators is becoming complex. The codebase already uses
twMergeinToolbarButton(as seen in relevant snippets). Using it here would improve readability and handle class conflicts properly.Proposed refactor using twMerge
+import { twMerge } from 'tailwind-merge'; // In the default variant branch: - className={`flex h-[32px] w-auto min-w-[32px] items-center justify-center rounded-md p-[5px] transition-colors ${ - active ? - 'border-none bg-blue-50 text-blue-500 shadow ring ring-blue-500' - : variantStyles.default - } ${disabled ? 'cursor-not-allowed opacity-50 hover:bg-transparent hover:ring-0' : 'cursor-pointer'} ${className}`} + className={twMerge( + 'flex h-[32px] w-auto min-w-[32px] items-center justify-center rounded-md p-[5px] transition-colors', + active + ? 'border-none bg-blue-50 text-blue-500 shadow ring ring-blue-500' + : variantStyles.default, + disabled + ? 'cursor-not-allowed opacity-50 hover:bg-transparent hover:ring-0' + : 'cursor-pointer', + className, + )}packages/web/src/lib/embedPdfEngine.js (1)
22-33: DuplicatedwithTimeoututility.This helper is identical to the one in
packages/web/src/lib/pdfUtils.js(lines 19-30). Consider extracting it to a shared utility module to avoid duplication.Suggested approach
Export
withTimeoutfrom a single location (e.g., keep it inpdfUtils.jsand import it here, or create a dedicatedasyncUtils.js):-function withTimeout(promise, ms, operationName = 'Operation') { - let timeoutId; - const timeoutPromise = new Promise((_, reject) => { - timeoutId = setTimeout(() => { - reject(new Error(`${operationName} timed out after ${ms / 1000} seconds`)); - }, ms); - }); - - return Promise.race([promise, timeoutPromise]).finally(() => { - clearTimeout(timeoutId); - }); -} +import { withTimeout } from './pdfUtils.js';packages/web/src/components/pdf/embedpdf/preact/src/components/command-button.tsx (1)
55-80: Nested ternaries reduce readability.The variant rendering logic uses deeply nested ternaries. Consider extracting to a helper function or using early returns for better maintainability.
Alternative approach using a render helper
function renderContent( variant: string, IconComponent: React.ComponentType<any> | null, iconProps: any, label: string ) { switch (variant) { case 'text': return <span className='text-sm'>{label}</span>; case 'icon-text': return ( <> {IconComponent && ( <IconComponent className={twMerge('mr-2 h-5 w-5', iconProps.className)} title={label} style={{ color: iconProps.primaryColor }} /> )} <span>{label}</span> </> ); case 'tab': return <span className='px-3 py-1'>{label}</span>; default: // 'icon' return IconComponent ? ( <IconComponent className={twMerge('h-5 w-5', iconProps.className)} title={label} style={{ color: iconProps.primaryColor, fill: iconProps.secondaryColor }} /> ) : ( <span>{label}</span> ); } }packages/web/src/lib/pdfUtils.js (1)
80-96: Inconsistent fallback logic for title extraction.The comment on line 89 says "Fallback: use first substantial line regardless of length check" but the code still applies length checks (
> 10and< 300). Additionally, the first loop (lines 81-87) already covers the first 20 lines with similar criteria.If both loops are intentional (different minimum lengths: 5 vs 10), consider clarifying the comment. If the fallback should truly ignore length checks, the code doesn't match the intent.
Clarified logic
// Find first substantial line (likely the title) - for (const line of lines.slice(0, 20)) { - // Check first 20 lines + for (const line of lines.slice(0, 20)) { // Scan header area const cleaned = cleanTitle(line); - if (cleaned.length > 5 && cleaned.length < 300) { + if (cleaned.length >= 10 && cleaned.length < 300) { return cleaned; } } - // Fallback: use first substantial line regardless of length check - for (const line of lines) { - const cleaned = cleanTitle(line); - if (cleaned.length > 10 && cleaned.length < 300) { - return cleaned; - } - } + // Fallback: accept shorter titles from anywhere in the document + for (const line of lines) { + const cleaned = cleanTitle(line); + if (cleaned.length >= 5 && cleaned.length < 300) { + return cleaned; + } + }packages/web/src/components/pdf/embedpdf/preact/src/viewer.tsx (1)
278-281: Consider making the fallback PDF URL configurable.The hardcoded external URL
https://snippet.embedpdf.com/ebook.pdfmight be intentional for demo purposes, but consider making it configurable via props or environment variables for better flexibility and to avoid unexpected external dependencies.packages/web/src/components/pdf/embedpdf/preact/src/config/commands.ts (1)
419-427: Placeholder implementation with console.log.The
document:propertiescommand contains a debug placeholder. Consider either implementing the functionality or throwing a "not implemented" error to make the incomplete state explicit.
| const handleImageLoad = () => { | ||
| // Clean up the object URL after image loads | ||
| if (urlRef.current) { | ||
| URL.revokeObjectURL(urlRef.current); | ||
| urlRef.current = null; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Premature URL revocation may break image display.
handleImageLoad revokes urlRef.current immediately after the image loads, but previewUrl state still holds the same URL. If the component re-renders (e.g., due to parent state change), the <img> may try to use the revoked URL, causing a broken image.
Consider keeping the URL valid until the dialog closes, or don't store the URL in both a ref and state.
Suggested simplification
Remove handleImageLoad and let handleClose handle all URL cleanup:
- const handleImageLoad = () => {
- // Clean up the object URL after image loads
- if (urlRef.current) {
- URL.revokeObjectURL(urlRef.current);
- urlRef.current = null;
- }
- };
...
<img
src={previewUrl}
- onLoad={handleImageLoad}
alt='Captured PDF area'
className='block max-h-[400px] max-w-full rounded border border-gray-200'
/>And update handleClose to clean up previewUrl:
const handleClose = () => {
// Clean up object URLs
if (urlRef.current) {
URL.revokeObjectURL(urlRef.current);
urlRef.current = null;
}
+ if (previewUrl) {
+ URL.revokeObjectURL(previewUrl);
+ }
if (downloadUrl) {
URL.revokeObjectURL(downloadUrl);
setDownloadUrl(null);
}
setOpen(false);
setCaptureData(null);
setPreviewUrl(null);
};Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
@packages/web/src/components/pdf/embedpdf/preact/src/components/capture-dialog.tsx
around lines 68-74, handleImageLoad revokes urlRef.current as soon as the image
loads which can break the <img> if the component re-renders because previewUrl
still points to the revoked object URL; remove the premature cleanup in
handleImageLoad (or delete handleImageLoad entirely) and instead perform URL
cleanup only in handleClose: revoke urlRef.current, set urlRef.current = null,
and clear previewUrl state there so the object URL remains valid for the
lifetime of the dialog; ensure you update any references to handleImageLoad so
the img onLoad no longer revokes the URL.
| const handleFullscreen = () => { | ||
| fullscreenProvider?.toggleFullscreen(`#${documentId}`); | ||
| setIsMenuOpen(false); | ||
| }; |
There was a problem hiding this comment.
Validate or sanitize documentId for use as CSS selector.
Line 48 constructs a CSS selector using template literal interpolation: `#${documentId}`. If the documentId contains special characters that are invalid in CSS selectors (such as spaces, colons, brackets, or other punctuation), the fullscreen toggle will fail. Consider either validating that documentId is a valid CSS identifier or using CSS.escape() to sanitize it.
Proposed fix using CSS.escape()
const handleFullscreen = () => {
- fullscreenProvider?.toggleFullscreen(`#${documentId}`);
+ fullscreenProvider?.toggleFullscreen(`#${CSS.escape(documentId)}`);
setIsMenuOpen(false);
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleFullscreen = () => { | |
| fullscreenProvider?.toggleFullscreen(`#${documentId}`); | |
| setIsMenuOpen(false); | |
| }; | |
| const handleFullscreen = () => { | |
| fullscreenProvider?.toggleFullscreen(`#${CSS.escape(documentId)}`); | |
| setIsMenuOpen(false); | |
| }; |
🤖 Prompt for AI Agents
In
@packages/web/src/components/pdf/embedpdf/preact/src/components/document-menu.tsx
around lines 47-50, handleFullscreen builds a CSS selector with `#${documentId}`
which can break if documentId contains characters invalid in selectors; update
handleFullscreen (and the call to fullscreenProvider?.toggleFullscreen) to
sanitize the id with CSS.escape(documentId) (or validate that documentId is a
valid CSS identifier) before constructing the selector, then call
fullscreenProvider?.toggleFullscreen(`#${escapedId}`) and finally
setIsMenuOpen(false).
| <button | ||
| onClick={() => provides?.closeDocument(documentState.id)} | ||
| disabled={isRetrying} | ||
| className='text-gray-400 hover:text-gray-600' | ||
| > | ||
| ✕ | ||
| </button> |
There was a problem hiding this comment.
Unicode symbol violates coding guidelines.
The close button uses the "✕" unicode character, which violates the project's coding guidelines that state "Do NOT use unicode symbols - unicode symbols are forbidden anywhere in the codebase."
Use an SVG icon instead, such as CloseIcon from the icons module.
Suggested fix
+import { AlertIcon, CloseIcon } from './icons';
-import { AlertIcon } from './icons';
...
<button
onClick={() => provides?.closeDocument(documentState.id)}
disabled={isRetrying}
className='text-gray-400 hover:text-gray-600'
>
- ✕
+ <CloseIcon className='h-5 w-5' />
</button>Based on coding guidelines.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
@packages/web/src/components/pdf/embedpdf/preact/src/components/document-password-prompt.tsx
around lines 71-77, Replace the unicode "✕" in the close button with the
project's SVG CloseIcon component: import CloseIcon from the icons module,
render <CloseIcon /> (or the project's equivalent) inside the button used for
provides?.closeDocument(documentState.id), keep the existing onClick and
disabled={isRetrying} behavior, and add an accessible label (e.g.,
aria-label="Close") or visually-hidden text so screen readers know the button
purpose.
| interface EmptyStateProps { | ||
| onDocumentOpened?: (_documentId: string) => void; | ||
| } |
There was a problem hiding this comment.
Remove underscore prefix from _documentId.
The underscore prefix conventionally indicates an unused parameter, but documentId is actively passed to the callback. This naming is misleading.
interface EmptyStateProps {
- onDocumentOpened?: (_documentId: string) => void;
+ onDocumentOpened?: (documentId: string) => void;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| interface EmptyStateProps { | |
| onDocumentOpened?: (_documentId: string) => void; | |
| } | |
| interface EmptyStateProps { | |
| onDocumentOpened?: (documentId: string) => void; | |
| } |
🤖 Prompt for AI Agents
In
@packages/web/src/components/pdf/embedpdf/preact/src/components/empty-state.tsx
around lines 3-5, Rename the parameter in the EmptyStateProps callback from
_documentId to documentId to reflect that the value is used; update the
interface declaration for onDocumentOpened (and any matching implementations or
usages of EmptyStateProps/onDocumentOpened) to accept documentId: string instead
of _documentId so the name is no longer misleading.
| export function LoadingSpinner({ size = 'md', message, className = '' }: LoadingSpinnerProps) { | ||
| return ( | ||
| <div className={`flex items-center gap-2 text-gray-600 ${className}`}> | ||
| <svg className={`${sizeClasses[size]} animate-spin`} fill='none' viewBox='0 0 24 24'> | ||
| <circle | ||
| className='opacity-25' | ||
| cx='12' | ||
| cy='12' | ||
| r='10' | ||
| stroke='currentColor' | ||
| strokeWidth='4' | ||
| /> | ||
| <path | ||
| className='opacity-75' | ||
| fill='currentColor' | ||
| d='M4 12a8 8 0 018-8V0C5.373 0 0 5.373 0 12h4zm2 5.291A7.962 7.962 0 014 12H0c0 3.042 1.135 5.824 3 7.938l3-2.647z' | ||
| /> | ||
| </svg> | ||
| {message && <span>{message}</span>} | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Add accessibility attributes for screen readers.
The loading spinner lacks ARIA attributes that are essential for users with screen readers. Loading states should be announced so users know content is being loaded.
Proposed accessibility enhancements
export function LoadingSpinner({ size = 'md', message, className = '' }: LoadingSpinnerProps) {
return (
- <div className={`flex items-center gap-2 text-gray-600 ${className}`}>
- <svg className={`${sizeClasses[size]} animate-spin`} fill='none' viewBox='0 0 24 24'>
+ <div className={`flex items-center gap-2 text-gray-600 ${className}`} role="status" aria-live="polite">
+ <svg className={`${sizeClasses[size]} animate-spin`} fill='none' viewBox='0 0 24 24' aria-hidden="true">
<circle
className='opacity-25'
cx='12'
cy='12'
r='10'
stroke='currentColor'
strokeWidth='4'
/>
<path
className='opacity-75'
fill='currentColor'
d='M4 12a8 8 0 018-8V0C5.373 0 0 5.373 0 12h4zm2 5.291A7.962 7.962 0 014 12H0c0 3.042 1.135 5.824 3 7.938l3-2.647z'
/>
</svg>
- {message && <span>{message}</span>}
+ {message ? <span>{message}</span> : <span className="sr-only">Loading...</span>}
</div>
);
}Note: Add a visually-hidden "Loading..." fallback when no message is provided to ensure screen readers always have context.
🤖 Prompt for AI Agents
In
@packages/web/src/components/pdf/embedpdf/preact/src/components/loading-spinner.tsx
around lines 13-34, The LoadingSpinner component is missing ARIA attributes and
a screen-reader fallback; update the LoadingSpinner function to add
role="status", aria-live="polite" and aria-busy="true" on the container, mark
the decorative SVG as aria-hidden="true", and render a visually-hidden text node
(e.g., "Loading...") when the message prop is not provided so screen readers
always get context; keep the visible message rendering unchanged and use the
existing sizeClasses/props to locate the component.
| return ( | ||
| <div className='flex h-full w-48 flex-col border-r border-gray-300 bg-gray-50'> | ||
| {/* Thumbnails */} | ||
| <div className='flex-1 overflow-hidden'> | ||
| <ThumbnailsPane documentId={documentId} style={{ width: '100%', height: '100%' }}> | ||
| {m => ( | ||
| <div | ||
| key={m.pageIndex} | ||
| style={{ | ||
| position: 'absolute', | ||
| width: '100%', | ||
| height: m.wrapperHeight, | ||
| top: m.top, | ||
| display: 'flex', | ||
| flexDirection: 'column', | ||
| alignItems: 'center', | ||
| cursor: 'pointer', | ||
| padding: '8px', | ||
| }} | ||
| onClick={() => { | ||
| provides?.scrollToPage?.({ | ||
| pageNumber: m.pageIndex + 1, | ||
| }); | ||
| }} | ||
| > | ||
| <div | ||
| style={{ | ||
| width: m.width, | ||
| height: m.height, | ||
| border: `2px solid ${state.currentPage === m.pageIndex + 1 ? '#3b82f6' : '#d1d5db'}`, | ||
| borderRadius: '4px', | ||
| overflow: 'hidden', | ||
| boxShadow: | ||
| state.currentPage === m.pageIndex + 1 ? | ||
| '0 0 0 2px rgba(59, 130, 246, 0.2)' | ||
| : 'none', | ||
| }} | ||
| > | ||
| <ThumbImg | ||
| documentId={documentId} | ||
| meta={m} | ||
| style={{ | ||
| width: '100%', | ||
| height: '100%', | ||
| objectFit: 'contain', | ||
| }} | ||
| /> | ||
| </div> | ||
| <div | ||
| style={{ | ||
| height: m.labelHeight, | ||
| display: 'flex', | ||
| alignItems: 'center', | ||
| justifyContent: 'center', | ||
| marginTop: '4px', | ||
| }} | ||
| > | ||
| <span className='text-xs text-gray-600'>{m.pageIndex + 1}</span> | ||
| </div> | ||
| </div> | ||
| )} | ||
| </ThumbnailsPane> | ||
| </div> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
Add keyboard navigation and ARIA attributes for accessibility.
The thumbnail items are clickable but lack keyboard navigation support and proper accessibility attributes. Users navigating with keyboards or screen readers cannot effectively interact with the thumbnails.
Proposed accessibility improvements
onClick={() => {
provides?.scrollToPage?.({
pageNumber: m.pageIndex + 1,
});
}}
+ onKeyDown={(e) => {
+ if (e.key === 'Enter' || e.key === ' ') {
+ e.preventDefault();
+ provides?.scrollToPage?.({
+ pageNumber: m.pageIndex + 1,
+ });
+ }
+ }}
+ role="button"
+ tabIndex={0}
+ aria-label={`Go to page ${m.pageIndex + 1}`}
+ aria-current={state.currentPage === m.pageIndex + 1 ? 'page' : undefined}
>🤖 Prompt for AI Agents
In
@packages/web/src/components/pdf/embedpdf/preact/src/components/thumbnails-sidebar.tsx
around lines 12-76, Thumbnail items lack keyboard and ARIA support; update the
clickable thumbnail container rendered inside ThumbnailsPane (the element using
key={m.pageIndex} and onClick calling provides?.scrollToPage) to be
keyboard-focusable and accessible by adding tabIndex=0, role="button" and
aria-pressed or aria-current (based on state.currentPage === m.pageIndex + 1),
and handle Enter/Space key events to invoke provides.scrollToPage({ pageNumber:
m.pageIndex + 1 }); also ensure ThumbImg has alt text or aria-hidden as
appropriate and that the page number span exposes its label (e.g.,
aria-label={`Page ${m.pageIndex + 1}`}), so screen readers and keyboard users
can interact with thumbnails.
| {/* <button | ||
| onClick={() => onModeChange('redact')} | ||
| className={`rounded px-4 py-1 text-sm font-medium transition-colors ${ | ||
| mode === 'redact' ? | ||
| 'bg-white text-gray-900 shadow-sm' | ||
| : 'text-gray-600 hover:text-gray-900' | ||
| }`} | ||
| > | ||
| Redact | ||
| </button> */} |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove or document the commented-out Redact button.
The Redact button is commented out, but the RedactionToolbar is still conditionally rendered (line 115), suggesting this feature is partially implemented. Either remove the commented code or add a comment explaining why it's temporarily disabled.
Proposed fix
If the feature is incomplete, remove the commented code:
- {/* <button
- onClick={() => onModeChange('redact')}
- className={`rounded px-4 py-1 text-sm font-medium transition-colors ${
- mode === 'redact' ?
- 'bg-white text-gray-900 shadow-sm'
- : 'text-gray-600 hover:text-gray-900'
- }`}
- >
- Redact
- </button> */}Or add a comment explaining the temporary state:
+ {/* Redact mode temporarily disabled pending UX review */}
{/* <button🤖 Prompt for AI Agents
In
@packages/web/src/components/pdf/embedpdf/preact/src/components/viewer-toolbar.tsx
around lines 89-98, The commented-out Redact button block should either be
removed or documented; locate the JSX block that calls onClick={() =>
onModeChange('redact')} and references mode === 'redact' (the commented Redact
button) and either delete that entire commented fragment or replace it with a
brief inline comment explaining why the button is disabled/hidden (e.g., feature
incomplete, gated behind flag) and reference that the RedactionToolbar
conditional render remains (RedactionToolbar) so future devs understand the
partial implementation.
| active: ({ state, documentId }) => | ||
| state.plugins['interaction-manager'].documents[documentId]?.activeMode === 'marqueeCapture', | ||
| }, |
There was a problem hiding this comment.
Potential null pointer exception - missing optional chaining.
Line 340 accesses state.plugins['interaction-manager'].documents without optional chaining, but this pattern is used elsewhere in the file (e.g., line 294, 316). If interaction-manager plugin is not registered, this will throw.
Proposed fix
active: ({ state, documentId }) =>
- state.plugins['interaction-manager'].documents[documentId]?.activeMode === 'marqueeCapture',
+ state.plugins['interaction-manager']?.documents[documentId]?.activeMode === 'marqueeCapture',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| active: ({ state, documentId }) => | |
| state.plugins['interaction-manager'].documents[documentId]?.activeMode === 'marqueeCapture', | |
| }, | |
| active: ({ state, documentId }) => | |
| state.plugins['interaction-manager']?.documents[documentId]?.activeMode === 'marqueeCapture', |
🤖 Prompt for AI Agents
In @packages/web/src/components/pdf/embedpdf/preact/src/config/commands.ts
around lines 339-341, The active checker may throw if the 'interaction-manager'
plugin is absent; update the expression in the active property to use optional
chaining for the plugin and documents lookup (e.g., change
state.plugins['interaction-manager'].documents[documentId]?.activeMode ===
'marqueeCapture' to
state.plugins['interaction-manager']?.documents?.[documentId]?.activeMode ===
'marqueeCapture') so it safely returns false when the plugin or documents is
undefined.
| import { ZOOM_PLUGIN_ID } from '@embedpdf/plugin-zoom'; | ||
| import { SEARCH_PLUGIN_ID } from '@embedpdf/plugin-search/react'; |
There was a problem hiding this comment.
Import path inconsistency with /react suffix.
SEARCH_PLUGIN_ID is imported from @embedpdf/plugin-search/react, but the rest of this Preact-based config module should use /preact imports for consistency.
Proposed fix
import { ZOOM_PLUGIN_ID } from '@embedpdf/plugin-zoom';
-import { SEARCH_PLUGIN_ID } from '@embedpdf/plugin-search/react';
+import { SEARCH_PLUGIN_ID } from '@embedpdf/plugin-search/preact';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { ZOOM_PLUGIN_ID } from '@embedpdf/plugin-zoom'; | |
| import { SEARCH_PLUGIN_ID } from '@embedpdf/plugin-search/react'; | |
| import { ZOOM_PLUGIN_ID } from '@embedpdf/plugin-zoom'; | |
| import { SEARCH_PLUGIN_ID } from '@embedpdf/plugin-search/preact'; |
🤖 Prompt for AI Agents
In @packages/web/src/components/pdf/embedpdf/preact/src/config/translations.ts
around lines 3-4, The import for SEARCH_PLUGIN_ID uses the React entry point but
this file is Preact-based; update the import from
'@embedpdf/plugin-search/react' to the Preact entry
'@embedpdf/plugin-search/preact' so it matches ZOOM_PLUGIN_ID's Preact usage
(leave ZOOM_PLUGIN_ID import as is), then run type checks/build to ensure no
other React-specific symbols are referenced.
| type SidebarState = { | ||
| search: boolean; | ||
| thumbnails: boolean; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove duplicate type definition.
The SidebarState type is already defined in packages/web/src/config/types.ts (lines 37-40). Import the existing type instead of redefining it locally to avoid potential inconsistencies.
Proposed fix
+import { SidebarState } from '@/config/types';
+
const logger = new ConsoleLogger();
-// Type for tracking sidebar state per document
-type SidebarState = {
- search: boolean;
- thumbnails: boolean;
-};
-
type ViewerPageProps = {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type SidebarState = { | |
| search: boolean; | |
| thumbnails: boolean; | |
| }; | |
| import { SidebarState } from '@/config/types'; | |
| const logger = new ConsoleLogger(); | |
| type ViewerPageProps = { |
🤖 Prompt for AI Agents
In @packages/web/src/components/pdf/embedpdf/preact/src/viewer.tsx around lines
61-64, The local SidebarState type declared in viewer.tsx duplicates the
existing SidebarState type defined in config types; remove the local type
declaration and import the SidebarState type from the shared types module, then
ensure all references in viewer.tsx use the imported SidebarState (update any
local usages or props/interfaces that referenced the removed declaration).
2 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.