package.json (2)

54-56: Review new UI devDependencies
The @headlessui/react and @radix-ui/react-alert-dialog libraries provide accessible UI components for the viewer toolbar and dialogs. Verify that these are tree-shakeable in your production build and that their bundle size impact is acceptable.

---

74-75: Inspect D3 modules and type definitions
The D3 packages (d3-drag, d3-selection) and their corresponding type definitions are introduced for interactive sidebar resizing in the PDF viewer. Confirm that only the needed D3 sub-modules are bundled (e.g., using selective imports) to minimize bundle size.

Also applies to: 83-84

src/main/modules/utils.ts (1)

70-77: Function duplication with existing waitFor.

The newly added sleep function duplicates the functionality of the existing waitFor function (lines 33-35). Consider consolidating these functions to avoid maintaining two identical utilities with different names.

You could either:

1. Use waitFor everywhere and remove sleep
2. Refactor waitFor to use sleep internally:

export function waitFor(ms: number) {
-  return new Promise((resolve) => setTimeout(resolve, ms));
+  return sleep(ms);
}

src/renderer/src/routes/pdf-viewer/page.tsx (1)

25-33: Consider adding loading states and error boundaries.

The App component would benefit from loading state management and error boundaries to handle PDF loading failures gracefully.

Wrap the Page component with an error boundary:

+import { ErrorBoundary } from 'react-error-boundary';

function App() {
  const ThemeProviderOrFragment = THEME_PROVIDER_ENABLED ? ThemeProvider : Fragment;

  return (
    <ThemeProviderOrFragment>
+      <ErrorBoundary fallback={<div className="p-4">Something went wrong loading the PDF viewer.</div>}>
        <Page />
+      </ErrorBoundary>
    </ThemeProviderOrFragment>
  );
}

src/renderer/src/routes/pdf-viewer/pdf-viewer/Toolbar/DocumentInfo.tsx (2)

22-34: Improve accessibility of the document info button.

The button lacks appropriate ARIA attributes for screen readers. Also, consider memoizing the click handler.

Enhance the button with accessibility attributes and optimize the event handler:

+  const handleOpenModal = useCallback(() => {
+    openModal();
+  }, []);

  return (
    <>
      <button
        className={`enabled:hover:bg-slate-200 enabled:hover:text-black text-slate-500 disabled:text-slate-300 p-1 rounded-sm transition-all group relative focus:border-blue-400 focus:ring-0 focus:shadow outline-none border border-transparent`}
-        onClick={() => {
-          openModal();
-        }}
+        onClick={handleOpenModal}
+        aria-label="Document Properties"
+        title="Document Properties"
      >
        <VscInfo className="w-4 h-4" />
        <Tooltip position="bottom" alignX="right">
          <p className="whitespace-nowrap">Document Properties</p>
        </Tooltip>
      </button>

      <DocumentInfoModal {...{ usePDFSlickStore, isOpen, closeModal }} />
    </>
  );

---

24-34: Consider extracting the button styles to a shared class.

The long inline class string could be refactored into a shared class or component for better maintainability.

Extract the button styles to a shared class or component:

+// In a shared styles file or component
+const toolbarButtonClass = `enabled:hover:bg-slate-200 enabled:hover:text-black text-slate-500 disabled:text-slate-300 p-1 rounded-sm transition-all group relative focus:border-blue-400 focus:ring-0 focus:shadow outline-none border border-transparent`;

// In this component
  return (
    <>
      <button
-        className={`enabled:hover:bg-slate-200 enabled:hover:text-black text-slate-500 disabled:text-slate-300 p-1 rounded-sm transition-all group relative focus:border-blue-400 focus:ring-0 focus:shadow outline-none border border-transparent`}
+        className={toolbarButtonClass}
        onClick={() => {
          openModal();
        }}
      >

src/renderer/src/routes/pdf-viewer/pdf-viewer/colors.ts (1)

1-211: Consider extracting this color palette to a shared location.

This comprehensive color palette appears to be a reusable resource that could benefit other parts of the application beyond the PDF viewer. Consider moving it to a shared UI utilities directory to promote reuse across the application.

Additionally, adding documentation comments explaining the color system's purpose and how to use it would make it easier for other developers to understand and utilize these colors correctly.

src/main/browser/sessions.ts (1)

101-101: Document why a 50ms sleep is necessary.

The code includes a 50ms delay before resolving the promise, but it's unclear why this specific timeout value is needed. Adding a comment explaining the purpose of this delay would help future developers understand why it's there and whether it can be safely modified.

-  // wait for 50 ms before returning
+  // A small delay is needed to ensure all session initialization is complete
+  // before other components that depend on the session can use it

src/renderer/src/routes/pdf-viewer/pdf-viewer/Thumbsbar/Outline.tsx (2)

23-28: Use a more accessible approach for hiding checkbox inputs.

Positioning elements thousands of pixels off-screen is a non-standard approach and may cause accessibility issues. Consider using the sr-only class from Tailwind or visually-hidden pattern.

-            className="peer absolute -top-[10000px] -left-[10000px]"
+            className="peer sr-only"

---

61-71: Add empty state handling and memoization for better performance.

The component doesn't handle the case when there's no outline data, and it could benefit from memoization to prevent unnecessary re-renders.

-const Outline = ({ usePDFSlickStore, show }: OutlineProps) => {
+import { useMemo } from "react";
+
+const Outline = ({ usePDFSlickStore, show }: OutlineProps) => {
   const documentOutline = usePDFSlickStore((s) => s.documentOutline);
   const pdfSlick = usePDFSlickStore((s) => s.pdfSlick);
+  
+  const outlineContent = useMemo(() => {
+    if (!documentOutline || documentOutline.length === 0) {
+      return (
+        <div className="p-4 text-center text-slate-500">
+          No outline available for this document.
+        </div>
+      );
+    }
+    
+    return renderOutlineItems({ outline: documentOutline, pdfSlick });
+  }, [documentOutline, pdfSlick]);

   return (
     <div className={clsx("overflow-auto absolute inset-0", { invisible: !show })}>
       <div className="p-2 pl-0.5 text-slate-700 text-sm">
-        {renderOutlineItems({ outline: documentOutline, pdfSlick })}
+        {outlineContent}
       </div>
     </div>
   );
 };

src/renderer/src/routes/pdf-viewer/pdf-viewer/Thumbsbar/Attachments.tsx (1)

31-43: Add empty state handling and improve accessibility.

The component doesn't handle the case when there are no attachments, which would provide a better user experience.

 const Attachments = ({ usePDFSlickStore, show }: AttachmentsProps) => {
   const attachments = usePDFSlickStore((s) => s.attachments);
+  const hasAttachments = attachments.size > 0;

   return (
     <div className={clsx("overflow-auto absolute inset-0", { invisible: !show })}>
       <div className="p-2 text-slate-700 text-sm">
-        {Array.from(attachments.entries()).map(([key, { filename, content }]) => (
-          <AttachmentButton key={key} {...{ usePDFSlickStore, filename, content }} />
-        ))}
+        {hasAttachments ? (
+          Array.from(attachments.entries()).map(([key, { filename, content }]) => (
+            <AttachmentButton key={key} {...{ usePDFSlickStore, filename, content }} />
+          ))
+        ) : (
+          <div className="p-4 text-center text-slate-500">
+            No attachments found in this document.
+          </div>
+        )}
       </div>
     </div>
   );
 };

src/renderer/src/routes/pdf-viewer/pdf-viewer/Toolbar/SearchBar.tsx (5)

17-17: Consider renaming state setter for consistency

The state variable is term but you're using setTerms for the setter. For consistency, consider renaming to setTerm.

- const [term, setTerms] = useState("");
+ const [term, setTerm] = useState("");

---

77-89: Add debouncing for search-as-you-type

The search is triggered on every input change, which can cause performance issues in large PDF documents. Consider implementing debouncing to reduce the number of search operations.

+ import { useCallback } from "react";
+ import debounce from "lodash/debounce";

// Inside component, before return:
+ const debouncedSearch = useCallback(
+   debounce((query) => {
+     pdfSlick?.eventBus.dispatch("find", {
+       type: "",
+       query,
+       caseSensitive,
+       highlightAll,
+       entireWord
+     });
+   }, 300),
+   [pdfSlick, caseSensitive, highlightAll, entireWord]
+ );

// Then in the onInput handler:
onInput={(e) => {
  const query = (e.target as HTMLInputElement).value;
-  pdfSlick?.eventBus.dispatch("find", {
-    type: "",
-    query,
-    caseSensitive,
-    highlightAll,
-    entireWord
-  });
+  debouncedSearch(query);
-  setTerms(query);
+  setTerm(query);
}}

---

21-35: Add support for F3 and Shift+F3 keyboard shortcuts

The component handles Cmd+F to open, but doesn't support common shortcuts like F3 or Shift+F3 for cycling through search results. Consider adding these for better user experience.

const handleEscape = (e: KeyboardEvent) => {
  let handled = false;

  if (e.key === "Escape") {
    setIsOpen(false);
  } else if (e.key === "f" && e.metaKey) {
    handled = true;
    setIsOpen(true);
+  } else if (e.key === "F3") {
+    handled = true;
+    if (term) {
+      pdfSlick?.eventBus.dispatch("find", {
+        type: "again",
+        query: term,
+        caseSensitive,
+        highlightAll,
+        entireWord,
+        findPrevious: e.shiftKey
+      });
+    }
  }

  if (handled) {
    e.preventDefault();
    return;
  }
};

---

88-88: Update function name to match state variable

For consistency with the state variable name (term), use setTerm instead of setTerms.

- setTerms(query);
+ setTerm(query);

---

54-64: Add ARIA attributes to improve accessibility

The search button lacks proper ARIA attributes for better screen reader support.

<button
  onClick={() => setIsOpen(!isOpen)}
+ aria-label="Search document"
+ aria-expanded={isOpen}
+ aria-controls="search-panel"
  className={`enabled:hover:bg-slate-200 enabled:hover:text-black  disabled:text-slate-300 p-1 rounded-sm transition-all group relative focus:border-blue-400 focus:ring-0 focus:shadow outline-none border border-transparent ${
    isOpen ? "bg-slate-200 text-black" : "text-slate-500"
  }`}
>
  <VscSearch className="h-4 w-4" />
</button>

{isOpen && (
  <div 
+   id="search-panel"
    className="absolute top-full mt-2 left-0 w-fit">

src/renderer/src/routes/pdf-viewer/pdf-viewer/Toolbar/MoreActionsMenu.tsx (4)

74-82: Remove unnecessary preventDefault calls

The e.preventDefault() calls in button click handlers are unnecessary since these are button click events, not form submissions.

onClick={() => pdfSlick?.requestPresentationMode()}

For the rotation handlers:

onClick={(e) => {
-  e.preventDefault();
  pdfSlick?.setRotation(pagesRotation + 90);
}}

onClick={(e) => {
-  e.preventDefault();
  pdfSlick?.setRotation(pagesRotation - 90);
}}

Also applies to: 174-177, 191-194

---

30-40: Consider adding error handling for Zustand store operations

The component accesses multiple properties from the Zustand store but doesn't handle cases where these might be undefined. Consider adding error handling or fallbacks.

const { pdfSlick, pagesRotation, pageNumber, numPages, scrollMode, spreadMode } = usePDFSlickStore(
  (s) => ({
    pdfSlick: s.pdfSlick,
    pagesRotation: s.pagesRotation ?? 0,
    pageNumber: s.pageNumber ?? 1,
    numPages: s.numPages ?? 1,
    scrollMode: s.scrollMode ?? ScrollMode.VERTICAL,
    spreadMode: s.spreadMode ?? SpreadMode.NONE
  }),
  shallow
);

---

69-361: Extract menu item rendering to reduce duplication

The menu items have a lot of duplicated code for styling and structure. Consider extracting a reusable menu item component to reduce duplication.

You could create a helper component like:

type MenuItemProps = {
  onClick: () => void;
  disabled?: boolean;
  icon: React.ReactNode;
  label: string;
  isActive?: boolean;
};

const MenuItem = ({ onClick, disabled, icon, label, isActive }: MenuItemProps) => {
  return (
    <Menu.Item>
      {({ active }) => (
        <button
          disabled={disabled}
          onClick={onClick}
          className={clsx(
            "w-full items-center flex space-x-2 box-border text-left px-2 py-1.5 text-xs disabled:opacity-50",
            {
              "bg-slate-100 text-gray-900": active,
              "text-gray-700": !active
            }
          )}
        >
          {icon}
          <span className="flex-1">{label}</span>
          {isActive && <VscCheck className="w-3" />}
        </button>
      )}
    </Menu.Item>
  );
};

Then use it like:

<MenuItem 
  onClick={() => pdfSlick?.requestPresentationMode()}
  icon={<VscPlay className="w-4 h-4" />}
  label="Presentation Mode"
/>

---

57-58: Add aria-haspopup attribute to improve accessibility

The menu button should include additional ARIA attributes for better accessibility.

<Menu.Button
  disabled={!pdfSlick}
+ aria-haspopup="true"
  className="enabled:hover:bg-slate-200 enabled:hover:text-black text-slate-500 disabled:text-slate-300 p-1 rounded-sm transition-all group relative focus:border-blue-400 focus:ring-0 focus:shadow outline-none border border-transparent"
>
  <span className="sr-only">Open more actions menu</span>
  <VscKebabVertical className="w-4 h-4" />
</Menu.Button>

src/renderer/src/routes/pdf-viewer/pdf-viewer/Tooltip.tsx (3)

10-62: Add a delay to tooltip display

The tooltip appears immediately on hover, which could lead to flickering when moving the mouse across multiple elements. Consider adding a small delay.

const Tooltip = ({ children, position = "right", alignX = "center" }: TooltipProps) => {
  return (
    <div
      role="tooltip"
      className={clsx(
        "absolute z-10 inline-block text-xs transition-opacity duration-300",
-       "rounded-sm shadow opacity-0 invisible",
+       "rounded-sm shadow opacity-0 invisible transition-all delay-300",
        "text-gray-500 bg-slate-50 border border-slate-300",
        "group-hover:visible group-hover:opacity-100",
        {
          "left-full top-1/2 -translate-y-1/2 ml-4": position === "right",
          "top-full mt-2": position === "bottom",
          "right-1/2 translate-x-1/2": position === "bottom" && alignX === "center",
          "right-0": position === "bottom" && alignX === "right",
          "left-0": position === "bottom" && alignX === "left"
        }
      )}
    >

---

4-8: Improve documentation of component usage

The Tooltip component depends on the parent element having a group class, but this isn't documented in the props interface.

type TooltipProps = {
  children: ReactNode;
  position?: "bottom" | "right";
  alignX?: "center" | "right" | "left";
+  /**
+   * Note: This component must be wrapped in a parent element with the 'group' class
+   * for the hover functionality to work properly.
+   */
};

---

1-64: Add mobile/touch device support

The tooltip relies on CSS hover, which doesn't work well on touch devices. Consider adding support for touch interactions.

To properly implement this, you would need to:

1. Add state to track whether the tooltip is visible
2. Add touch event handlers to toggle the tooltip
3. Use the state to conditionally apply classes instead of just CSS hover

Basic implementation:

import { ReactNode, useState } from "react";
import clsx from "clsx";

type TooltipProps = {
  children: ReactNode;
  position?: "bottom" | "right";
  alignX?: "center" | "right" | "left";
};

const Tooltip = ({ children, position = "right", alignX = "center" }: TooltipProps) => {
  const [isVisible, setIsVisible] = useState(false);
  
  return (
    <div
      role="tooltip"
      className={clsx(
        "absolute z-10 inline-block text-xs transition-opacity duration-300",
        "rounded-sm shadow opacity-0 invisible",
        "text-gray-500 bg-slate-50 border border-slate-300",
        {
          "opacity-100 visible": isVisible,
          "group-hover:visible group-hover:opacity-100": true,
          "left-full top-1/2 -translate-y-1/2 ml-4": position === "right",
          "top-full mt-2": position === "bottom",
          "right-1/2 translate-x-1/2": position === "bottom" && alignX === "center",
          "right-0": position === "bottom" && alignX === "right",
          "left-0": position === "bottom" && alignX === "left"
        }
      )}
    >
      {/* ... rest of component */}
    </div>
  );
};

src/renderer/src/routes/pdf-viewer/pdf-viewer/Toolbar/index.tsx (4)

83-89: Strengthen page number validation

The page number validation is good, but could be more robust by using a regex check for numeric input before parsing.

onSubmit={(e) => {
  e.preventDefault();
- const newPageNumber = parseInt(wantedPageNumber + "");
- if (Number.isInteger(newPageNumber) && newPageNumber > 0 && newPageNumber <= numPages) {
+ const pageStr = String(wantedPageNumber);
+ if (/^\d+$/.test(pageStr)) {
+   const newPageNumber = parseInt(pageStr, 10);
+   if (newPageNumber > 0 && newPageNumber <= numPages) {
      pdfSlick?.linkService.goToPage(newPageNumber);
+   } else {
+     setWantedPageNumber(pageNumber);
+   }
  } else {
    setWantedPageNumber(pageNumber);
  }
}}

---

35-35: Add type safety to updatePageNumber function

The updatePageNumber function should have stronger typing to ensure pageNumber is always a number.

- const updatePageNumber = ({ pageNumber }: { pageNumber: number }) => setWantedPageNumber(pageNumber);
+ const updatePageNumber = ({ pageNumber }: { pageNumber: number }) => setWantedPageNumber(pageNumber || 1);

---

125-149: Implement a more responsive layout for smaller screens

Some UI elements are already hidden on small screens (sm breakpoint), but the toolbar could still be crowded on very small screens. Consider implementing a more responsive design that collapses more elements into the "More" menu at smaller sizes.

You could:

1. Move more elements into the MoreActionsMenu component
2. Use additional responsive breakpoints (like md or lg)
3. Consider implementing a collapsible toolbar that shows only essential controls initially

For example:

<div className="px-1 space-x-1 flex items-center justify-end">
- <FreetextMenu {...{ usePDFSlickStore }} />
- <InkMenu {...{ usePDFSlickStore }} />
+ <div className="hidden xs:flex items-center space-x-1">
+   <FreetextMenu {...{ usePDFSlickStore }} />
+   <InkMenu {...{ usePDFSlickStore }} />
+ </div>

  <Splitter />

  <div className="items-center space-x-1 hidden sm:flex">

And then add these elements to the MoreActionsMenu when they're hidden.

---

46-55: Add tooltip to sidebar toggle button

The sidebar toggle button lacks a tooltip, unlike other buttons in the toolbar. For consistency, add a tooltip.

<button
  className={`enabled:hover:bg-slate-200 enabled:hover:text-black text-slate-500 disabled:text-slate-300 p-1 rounded-sm transition-all group relative focus:border-blue-400 focus:ring-0 focus:shadow outline-none border border-transparent`}
  onClick={() => setIsThumbsbarOpen(!isThumbsbarOpen)}
>
  {isThumbsbarOpen ? (
    <VscLayoutSidebarLeftOff className="h-4 w-4" />
  ) : (
    <VscLayoutSidebarLeft className="h-4 w-4" />
  )}
+ <Tooltip position="bottom">
+   <p className="whitespace-nowrap">{isThumbsbarOpen ? "Hide sidebar" : "Show sidebar"}</p>
+ </Tooltip>
</button>

src/renderer/src/routes/pdf-viewer/pdf-viewer/Toolbar/DocumentInfoModal.tsx (2)

108-115: Consider using dynamic locale instead of hardcoded "en-US".

The date formatting currently uses a hardcoded "en-US" locale. Consider using the user's system locale for a more localized experience:

-new Intl.DateTimeFormat("en-US", {
+new Intl.DateTimeFormat(navigator.language || "en-US", {

Also applies to: 122-129

---

62-180: Consider extracting repeated property display pattern.

There's significant repetition in the property display pattern. Consider extracting this into a reusable component to improve maintainability:

+type PropertyRowProps = {
+  label: string;
+  value: React.ReactNode;
+};
+
+const PropertyRow = ({ label, value }: PropertyRowProps) => (
+  <div className="grid grid-cols-3 gap-4 px-6 py-1 my-0.5 hover:bg-slate-50">
+    <dt className="text-sm font-medium text-gray-500">{label}</dt>
+    <dd className="text-sm text-gray-900 col-span-2">
+      {value ?? <span className="text-xs text-gray-500">N/A</span>}
+    </dd>
+  </div>
+);

Then use it throughout the component.

src/renderer/src/routes/pdf-viewer/pdf-viewer/Thumbsbar/ButtonsBar.tsx (1)

28-32: Extract common button styles.

All three buttons share similar styling logic. Consider extracting the common styles to reduce duplication:

+const buttonBaseClasses = "enabled:hover:bg-slate-200 enabled:hover:text-black text-slate-500 disabled:text-slate-300 p-1 rounded-sm transition-all group relative focus:border-blue-400 focus:ring-0 focus:shadow outline-none border border-transparent";
+const buttonActiveClasses = "enabled:bg-blue-100 enabled:text-black border-blue-200";

// Then use them like:
className={clsx(
  buttonBaseClasses,
  {
    [buttonActiveClasses]: tab === "thumbnails"
  }
)}

Also applies to: 43-47, 59-63

src/renderer/src/routes/pdf-viewer/pdf-viewer/index.tsx (1)

45-49: Improve progress indicator appearance.

The current progress bar is very thin (h-px) which might be hard to see on high-DPI displays. Consider making it slightly taller and adding a subtle animation:

<div
-  className="fixed top-0 left-0 h-px bg-blue-600 z-50 transition-all duration-150 ease-out"
+  className="fixed top-0 left-0 h-1 bg-blue-600 z-50 transition-all duration-150 ease-out animate-pulse"
  style={{ width: `${loadedPerc}%` }}
></div>

src/renderer/src/routes/pdf-viewer/pdf-viewer/Toolbar/ZoomSelector.tsx (1)

49-49: Replace bitwise operator with more readable Math function.

The code uses the bitwise NOT operator twice (~~) as a clever way to round numbers, but this reduces readability. Consider using standard Math methods:

- {scaleValue && presets.has(scaleValue) ? presets.get(scaleValue) : `${~~(scale * 100)}%`}
+ {scaleValue && presets.has(scaleValue) ? presets.get(scaleValue) : `${Math.floor(scale * 100)}%`}

src/main/browser/utility/intercept-rules.ts (2)

13-14: Consider using the modern URL API instead of deprecated URL.parse

The code is using the deprecated URL.parse() method. It's recommended to use the modern URL constructor instead.

- const protocol = URL.parse(currentUrl ?? "")?.protocol;
+ const protocol = new URL(currentUrl ?? "").protocol;

---

48-52: Add early return pattern for improved readability

The code could benefit from using an early return pattern when the URL object is null, which would make the logic clearer and reduce nesting.

  const url = details.url;
  const urlObject = URL.parse(url);
  if (!urlObject) {
-   return callback({});
+   callback({});
+   return;
  }

src/renderer/src/routes/pdf-viewer/pdf-viewer/Toolbar/FreetextMenu.tsx (1)

54-55: Remove commented code

There are commented-out <Menu.Item> tags that should be removed to maintain clean code.

- // <Menu.Item key={`${name}-${s}`}>
  <button
    key={`${name}-${s}`}
    className="p-2 rounded-full border border-slate-200 hover:border-slate-400 hover:shadow-sm focus:ring-blue-500"
    style={{
      backgroundColor: colors[name][s]
    }}
    onClick={() => {
      pdfSlick?.setAnnotationEditorMode(AnnotationEditorType.FREETEXT);
      pdfSlick?.setAnnotationEditorParams([
        {
          type: AnnotationEditorParamsType.FREETEXT_COLOR,
          value: colors[name][s]
        }
      ]);
    }}
  />
- // </Menu.Item>

Also applies to: 71-72

src/renderer/src/routes/pdf-viewer/pdf-viewer/Thumbsbar/index.tsx (1)

23-36: Extract magic numbers to named constants

The code uses magic numbers (233 and 620) for the minimum and maximum width constraints. It would be better to define these as named constants at the top of the component for better readability and maintainability.

const Thumbsbar = ({ usePDFSlickStore, isThumbsbarOpen, thumbsRef }: ThumbsbarProps) => {
  const [tab, setTab] = useState<"thumbnails" | "outline" | "attachments">("thumbnails");

+  const MIN_SIDEBAR_WIDTH = 233;
+  const MAX_SIDEBAR_WIDTH = 620;
+
  const containerRef = useRef<HTMLDivElement>(null);
  const resizerRef = useRef<HTMLDivElement>(null);
  const [isResizing, setIsResizing] = useState(false);
- const [width, setWidth] = useState(233);
+ const [width, setWidth] = useState(MIN_SIDEBAR_WIDTH);

  useEffect(() => {
    let newWidth = 0;

    const dragResize = drag<HTMLDivElement, unknown>()
      .on("start", () => {
        newWidth = containerRef.current?.clientWidth ?? 0;
        setIsResizing(true);
      })
      .on("drag", (e) => {
        newWidth += e.dx;
-       const width = Math.min(620, Math.max(233, newWidth));
+       const width = Math.min(MAX_SIDEBAR_WIDTH, Math.max(MIN_SIDEBAR_WIDTH, newWidth));
        setWidth(width);
      })

src/renderer/src/routes/pdf-viewer/pdf-viewer/Thumbsbar/Thumbnails.tsx (2)

40-41: Shadowing width/height causes mental overhead

({ pageNumber, width, height, … }) shadows the outer width from useMeasure, making the code harder to scan. A small rename improves readability:

-          {({ pageNumber, width, height, src, pageLabel, loaded }) => (
+          {({ pageNumber, width: thumbW, height: thumbH, src, pageLabel, loaded }) => (
…
-                        width: `${width + 2}px`,
-                        height: `${height + 2}px`
+                        width: `${thumbW + 2}px`,
+                        height: `${thumbH + 2}px`
…
-                      {src && <img src={src} width={width} height={height} />}
+                      {src && <img src={src} width={thumbW} height={thumbH} />}

---

25-29: Minor ref forwarding ergonomics

ref(el!) will throw if el is null (when unmounting) because the non-null assertion forces the type. Forwarding the value as-is avoids the unnecessary assertion:

-      ref={(el) => {
-        ref(el!);
-      }}
+      ref={ref}

This works because the ref function returned by useMeasure matches the expected signature.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting

src/renderer/src/routes/pdf-viewer/pdf-viewer/Toolbar/MoreActionsMenu.tsx (1)

343-343: Fix incorrect button disable condition

The "Document Properties" button is incorrectly disabled when on the last page. Document properties should be accessible regardless of the current page.

-disabled={pageNumber === numPages}

src/renderer/src/routes/pdf-viewer/pdf-viewer/Toolbar/MoreActionsMenu.tsx (6)

73-85: Consider adding keyboard shortcuts for quick access

The presentation mode button (and other frequently used actions) would benefit from keyboard shortcuts to improve user experience and accessibility.

Add a keyboard shortcut indicator to the presentation mode button:

 <button
   onClick={() => pdfSlick?.requestPresentationMode()}
   className={clsx(
     "w-full items-center flex space-x-2 box-border text-left px-2 py-1.5 text-xs disabled:opacity-50",
     {
       "bg-slate-100 text-gray-900": active,
       "text-gray-700": !active
     }
   )}
 >
   <VscPlay className="w-4 h-4" />
-  <span>Presentation Mode</span>
+  <span className="flex-1">Presentation Mode</span>
+  <span className="text-xs text-gray-500">F11</span>
 </button>

---

27-40: Optimize component rendering with useCallback

Event handlers are recreated on each render, which can impact performance. Using useCallback would maintain referential equality between renders.

 const MoreActionsMenu = ({ usePDFSlickStore }: MoreActionsMenuProps) => {
   const [isOpen, setIsOpen] = useState(false);
 
   const { pdfSlick, pagesRotation, pageNumber, numPages, scrollMode, spreadMode } = usePDFSlickStore(
     (s) => ({
       pdfSlick: s.pdfSlick,
       pagesRotation: s.pagesRotation,
       pageNumber: s.pageNumber,
       numPages: s.numPages,
       scrollMode: s.scrollMode,
       spreadMode: s.spreadMode
     }),
     shallow
   );
+
+  const handleSave = useCallback(() => pdfSlick?.downloadOrSave(), [pdfSlick]);
+  const handlePrint = useCallback(() => pdfSlick?.triggerPrinting(), [pdfSlick]);
+  const handleRotateClockwise = useCallback(() => pdfSlick?.setRotation(pagesRotation + 90), [pdfSlick, pagesRotation]);

Then use these callbacks in your click handlers.

---

50-69: Enhance menu accessibility with ARIA attributes

The menu button has good screen reader support, but the menu items could benefit from additional ARIA attributes for improved accessibility.

Add ARIA attributes to the menu component:

   return (
     <>
-      <Menu as="span" className="">
+      <Menu as="span" className="" aria-label="PDF viewer options">
         <Menu.Button
           disabled={!pdfSlick}
           className="enabled:hover:bg-slate-200 enabled:hover:text-black text-slate-500 disabled:text-slate-300 p-1 rounded-sm transition-all group relative focus:border-blue-400 focus:ring-0 focus:shadow outline-none border border-transparent"
+          aria-haspopup="true"
         >
           <span className="sr-only">Open more actions menu</span>
           <VscKebabVertical className="w-4 h-4" />
         </Menu.Button>
         <Transition
           as={Fragment}
           enter="transition ease-out duration-100"
           enterFrom="transform opacity-0 scale-95"
           enterTo="transform opacity-100 scale-100"
           leave="transition ease-in duration-75"
           leaveFrom="transform opacity-100 scale-100"
           leaveTo="transform opacity-0 scale-95"
         >
-          <Menu.Items className="absolute right-2 w-52 z-30 mt-2 origin-top-right divide-y divide-slate-200 rounded text-left bg-white shadow-lg ring-1 ring-black/5 focus:outline-none">
+          <Menu.Items className="absolute right-2 w-52 z-30 mt-2 origin-top-right divide-y divide-slate-200 rounded text-left bg-white shadow-lg ring-1 ring-black/5 focus:outline-none" aria-orientation="vertical" role="menu">

---

74-106: Extract repeated menu item pattern into a reusable component

There's significant code duplication in the structure and styling of menu items. Extracting this pattern would improve maintainability.

Create a reusable menu item component:

type MenuItemProps = {
  icon: React.ReactNode;
  label: string;
  onClick: () => void;
  disabled?: boolean;
  isActive?: boolean;
  rightElement?: React.ReactNode;
};

const MenuItem = ({ icon, label, onClick, disabled = false, rightElement }: MenuItemProps) => (
  <Menu.Item>
    {({ active }) => (
      <button
        onClick={onClick}
        disabled={disabled}
        className={clsx(
          "w-full items-center flex space-x-2 box-border text-left px-2 py-1.5 text-xs disabled:opacity-50",
          {
            "bg-slate-100 text-gray-900": active,
            "text-gray-700": !active
          }
        )}
      >
        {icon}
        <span className="flex-1">{label}</span>
        {rightElement}
      </button>
    )}
  </Menu.Item>
);

Then use it in the menu:

<MenuItem 
  icon={<VscDesktopDownload className="w-4 h-4" />}
  label="Save"
  onClick={() => pdfSlick?.downloadOrSave()}
/>

---

173-183: Improve handling of rotation functions

The current implementation directly sets rotation angles, but doesn't normalize them to stay within 0-359 degrees, which could lead to unnecessarily large values after multiple rotations.

Consider normalizing the rotation value:

   <button
     onClick={(e) => {
       e.preventDefault();
-      pdfSlick?.setRotation(pagesRotation + 90);
+      pdfSlick?.setRotation((pagesRotation + 90) % 360);
     }}
     className={clsx("w-full flex space-x-2 box-border text-left px-2 py-1.5 text-xs", {
       "bg-slate-100 text-gray-900": active,
       "text-gray-700": !active
     })}
   >
     <VscRefresh className="w-4 h-4" />

---

23-25: Type definition for usePDFSlickStore could be more specific

The current type TUsePDFSlickStore is imported from a third-party library, but it might be beneficial to define more specific types for your use case.

Consider creating a more specific type that only includes the properties you're using:

+import { type StoreApi, type UseBoundStore } from 'zustand';
+
+interface PDFSlickState {
+  pdfSlick: any; // Replace with more specific type if available
+  pagesRotation: number;
+  pageNumber: number;
+  numPages: number;
+  scrollMode: ScrollMode;
+  spreadMode: SpreadMode;
+}
+
 type MoreActionsMenuProps = {
-  usePDFSlickStore: TUsePDFSlickStore;
+  usePDFSlickStore: UseBoundStore<StoreApi<PDFSlickState>>;
 };

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting

src/main/browser/utility/intercept-rules.ts (1)

43-58: ⚠️ Potential issue

Security issue: Missing URL scheme validation

The code only checks if the pathname ends with ".pdf" but doesn't validate the URL scheme, which could be a security risk if a malicious URL with a PDF extension is processed.

Implement secure URL validation by checking the protocol:

  const { pathname } = urlObject;
- if (pathname && pathname.toLowerCase().endsWith(".pdf") && getSettingValueById("enableFlowPdfViewer") === true) {
+ // Only redirect HTTP/HTTPS URLs or other trusted protocols
+ const isValidProtocol = url.startsWith('http:') || url.startsWith('https:') || url.startsWith('file:');
+ if (pathname && pathname.toLowerCase().endsWith(".pdf") && isValidProtocol && getSettingValueById("enableFlowPdfViewer") === true) {

🧰 Tools

🪛 Biome (1.9.4)

[error] 57-57: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

src/main/browser/utility/intercept-rules.ts (2)

57-57: Use optional chaining for code safety

The static analysis hint suggests using optional chaining here for better safety.

- if (pathname && pathname.toLowerCase().endsWith(".pdf") && getSettingValueById("enableFlowPdfViewer") === true) {
+ if (pathname?.toLowerCase().endsWith(".pdf") && getSettingValueById("enableFlowPdfViewer") === true) {

🧰 Tools

🪛 Biome (1.9.4)

[error] 57-57: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

---

90-101: Origin header handling may be too broad

The function is replacing the Origin header for all requests, not just PDF-related ones. This could potentially affect other requests unnecessarily.

Consider limiting the Origin header modification to only PDF-related requests:

  webRequest.onBeforeSendHeaders((details, callback) => {
    const url = details.url;
    const urlObject = URL.parse(url);
    if (!urlObject) {
      return callback({});
    }

+   // Only modify headers for PDF-related URLs
+   const { pathname } = urlObject;
+   if (pathname?.toLowerCase().endsWith(".pdf")) {
      const newHeaders = { ...details.requestHeaders, Origin: urlObject.origin };
      callback({ requestHeaders: newHeaders });
+     return;
+   }

    callback({});
  });

src/main/browser/utility/protocols/_protocols/flow-external.ts (1)

29-36: Misleading comment in protocol handler

The comment // flow://:path doesn't match the actual protocol being handled (flow-external).

  protocol.handle("flow-external", async (request) => {
    const urlString = request.url;
    const url = new URL(urlString);

-   // flow://:path
+   // flow-external://:path
    return await handleDomainRequest(request, url);
  });

src/main/browser/utility/protocols/utils.ts (3)

39-47: Well-implemented development server integration

The code properly checks for development mode with hot reload enabled and attempts to serve files from the development server when appropriate.

Consider adding a timeout for the fetch operation to prevent hanging in case the development server is slow to respond.

 if (ping) {
-      return await fetchFromDevServer(transformedPath, request);
+      // Add timeout to prevent hanging if dev server is slow
+      return await Promise.race([
+        fetchFromDevServer(transformedPath, request),
+        new Promise<Response>((_, reject) => 
+          setTimeout(() => reject(new Error('Dev server timeout')), 5000)
+        ).catch(error => {
+          console.warn('Dev server timeout:', error);
+          return new Response('Development server timeout', { status: 504 });
+        })
+      ]);
 }

---

49-65: Well-structured file serving implementation

The code correctly checks for directories, reads file contents, determines appropriate content types, and returns proper responses.

Consider adding caching headers to improve performance for static assets:

 return new Response(buffer, {
   headers: {
-        "Content-Type": contentType
+        "Content-Type": contentType,
+        "Cache-Control": "public, max-age=86400"
   }
 });

---

66-69: Error handling could be more specific

The current implementation logs the error and returns a generic 404 response for all error types.

Consider differentiating between common error types for better debugging and user experience:

 } catch (error) {
   console.error("Error serving file:", error);
-  return new Response("File not found", { status: 404 });
+  if (error.code === 'ENOENT') {
+    return new Response("File not found", { status: 404 });
+  } else if (error.code === 'EACCES') {
+    return new Response("Permission denied", { status: 403 });
+  } else {
+    return new Response("Internal server error", { status: 500 });
+  }
 }

src/main/browser/utility/protocols/_protocols/flow.ts (3)

22-34: PDF caching implementation needs documentation

The PDF cache implementation is functional but lacks explanatory comments.

Add documentation to explain the purpose and lifecycle of cached PDF responses:

+/**
+ * In-memory cache for PDF responses.
+ * Used to temporarily store PDF content that needs to be displayed
+ * in the PDF viewer without re-downloading or processing it again.
+ */
 const PDF_CACHE = new Map<string, Response>();

+/**
+ * Adds a PDF response to the cache with the specified key.
+ * @param key - Unique identifier for the cached response
+ * @param response - The PDF response to cache
+ */
 export function addPdfResponseToCache(key: string, response: Response) {
   PDF_CACHE.set(key, response);
 }

+/**
+ * Retrieves a PDF response from the cache.
+ * @param key - The key to look up
+ * @returns The cached response or undefined if not found
+ */
 function getPdfResponseFromCache(key: string): Response | undefined {
   return PDF_CACHE.get(key);
 }

+/**
+ * Removes a PDF response from the cache.
+ * @param key - The key of the response to remove
+ */
 function removePdfResponseFromCache(key: string) {
   PDF_CACHE.delete(key);
 }

---

82-98: Asset serving error handling could be improved

The error handling for asset requests is basic but functional.

Consider improving error handling with more specific error types and adding logging of the specific asset path for debugging:

 try {
   // Read file contents
   const buffer = await fsPromises.readFile(filePath);

   // Determine content type based on file extension
   const contentType = getContentType(filePath);

   return new Response(buffer, {
     headers: {
       "Content-Type": contentType
     }
   });
 } catch (error) {
-  console.error("Error serving asset:", error);
+  console.error(`Error serving asset '${normalizedPath}':`, error);
+  
+  if (error.code === 'ENOENT') {
     return new Response("Asset not found", { status: 404 });
+  } else if (error.code === 'EACCES') {
+    return new Response("Permission denied", { status: 403 });
+  } else {
+    return new Response("Internal server error", { status: 500 });
+  }
 }

---

135-150: PDF cache request handler should document cleanup

The PDF cache handler removes the response from cache after retrieving it, but this behavior isn't obvious.

Add a comment explaining the one-time use nature of the cached PDF responses:

 const handlePdfCacheRequest = async (_request: Request, url: URL) => {
   const pdfURL = url.searchParams.get("url");
   const key = url.searchParams.get("key");
   if (!pdfURL || !key) {
     return new Response("Invalid request path", { status: 400 });
   }

   const pdfResponse = getPdfResponseFromCache(key);
   if (!pdfResponse) {
     // redirect to actual url
     return Response.redirect(pdfURL);
   }

+  // Remove from cache after use - PDFs are meant to be retrieved only once
   removePdfResponseFromCache(key);
   return pdfResponse;
 };

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting