From 567b29abf28b98cd255b4a1164b195a59198b099 Mon Sep 17 00:00:00 2001
From: Sunny <sunny@foodready.ai>
Date: Mon, 2 Feb 2026 04:48:16 +0000
Subject: [PATCH] fix: address CodeRabbit review comments for PR #76

Security fixes:
- Validate returnTo parameter in auth route to prevent open redirect attacks

Performance fixes:
- Add concurrency limit (3) for EPUB spine extraction to prevent memory exhaustion
- Iteratively split oversized paragraphs in chunk splitting to prevent content loss
- Add iteration limit (10) and shrink-check to summary combining loop

Data integrity:
- Remap summary rows when document IDs change during sync

UX improvements:
- Clear summary base URL when switching to non-custom providers
- Add type="button" to SummarizeButton to prevent form submission
- Add dev logging to PDFViewer error handler
- Move document state detection from render to useEffect

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
---
 src/app/api/auth/route.ts          | 21 ++++++++++-
 src/app/epub/[id]/page.tsx         | 42 ++++++++++++++--------
 src/components/PDFViewer.tsx       | 21 ++++++-----
 src/components/SettingsModal.tsx   |  4 +++
 src/components/SummarizeButton.tsx |  1 +
 src/lib/concurrency.ts             | 50 ++++++++++++++++++++++++++
 src/lib/dexie.ts                   | 19 ++++++++++
 src/lib/summarize.ts               | 57 +++++++++++++++++++++++++++---
 8 files changed, 186 insertions(+), 29 deletions(-)
 create mode 100644 src/lib/concurrency.ts

diff --git a/src/app/api/auth/route.ts b/src/app/api/auth/route.ts
index 94abe7d..6e28f30 100644
--- a/src/app/api/auth/route.ts
+++ b/src/app/api/auth/route.ts
@@ -1,6 +1,23 @@
 import { NextRequest, NextResponse } from 'next/server';
 import { getAuthToken } from '@/lib/auth';
 
+function isValidReturnTo(returnTo: string): boolean {
+  // Only allow relative paths starting with /
+  // Reject absolute URLs, protocol-relative URLs, and other schemes
+  if (!returnTo.startsWith('/')) return false;
+  if (returnTo.startsWith('//')) return false;
+  // Reject URLs with encoded characters that could bypass checks
+  if (returnTo.includes('%')) {
+    try {
+      const decoded = decodeURIComponent(returnTo);
+      if (decoded.startsWith('//') || decoded.includes('://')) return false;
+    } catch {
+      return false;
+    }
+  }
+  return true;
+}
+
 export async function GET(request: NextRequest) {
   const token = request.nextUrl.searchParams.get('token');
   const returnTo = request.nextUrl.searchParams.get('returnTo') || '/';
@@ -9,7 +26,9 @@ export async function GET(request: NextRequest) {
     return NextResponse.json({ error: 'Invalid token' }, { status: 401 });
   }
 
-  const response = NextResponse.redirect(new URL(returnTo, request.url));
+  // Validate returnTo to prevent open redirect attacks
+  const safeReturnTo = isValidReturnTo(returnTo) ? returnTo : '/';
+  const response = NextResponse.redirect(new URL(safeReturnTo, request.url));
   response.cookies.set('auth_session', token, {
     httpOnly: true,
     secure: process.env.NODE_ENV === 'production',
diff --git a/src/app/epub/[id]/page.tsx b/src/app/epub/[id]/page.tsx
index 6f02013..2029b58 100644
--- a/src/app/epub/[id]/page.tsx
+++ b/src/app/epub/[id]/page.tsx
@@ -20,6 +20,7 @@ import { SummarizeButton } from '@/components/SummarizeButton';
 import { SummarizeModal } from '@/components/SummarizeModal';
 import type { SummarizeMode } from '@/types/summary';
 import { resolveDocumentId } from '@/lib/dexie';
+import { processWithConcurrencyLimit } from '@/lib/concurrency';
 
 const isDev = process.env.NEXT_PUBLIC_NODE_ENV !== 'production' || process.env.NODE_ENV == null;
 
@@ -126,27 +127,38 @@ export default function EPUBPage() {
     }
 
     if (mode === 'whole_book') {
-      // Extract text from all spine sections
-      const promises: Promise<string>[] = [];
+      // Extract text from all spine sections with concurrency limit to prevent memory exhaustion
+      const spineItems: { href: string }[] = [];
       const spine = book.spine;
 
       spine.each((item: { href?: string }) => {
         const url = item.href || '';
-        if (!url) return;
-
-        const promise = book.load(url)
-          .then((section) => (section as Document))
-          .then((section) => section.body?.textContent?.trim() || '')
-          .catch((err) => {
-            console.warn('Failed to extract text from section:', url, err);
-            return '';
-          });
-
-        promises.push(promise);
+        if (url) {
+          spineItems.push({ href: url });
+        }
       });
 
-      const textParts = await Promise.all(promises);
-      return textParts.filter(text => text).join('\n\n');
+      // Use concurrency limit of 3 to prevent memory spikes on large books
+      const results = await processWithConcurrencyLimit(
+        spineItems,
+        async (item) => {
+          try {
+            const section = await book.load(item.href) as Document;
+            return section.body?.textContent?.trim() || '';
+          } catch (err) {
+            console.warn('Failed to extract text from section:', item.href, err);
+            return '';
+          }
+        },
+        3 // Max concurrent extractions
+      );
+
+      const textParts = results
+        .filter((r): r is { status: 'fulfilled'; value: string } => r.status === 'fulfilled')
+        .map(r => r.value)
+        .filter(text => text);
+
+      return textParts.join('\n\n');
     } else {
       // Extract current page text
       return extractPageText(book, rendition, false);
diff --git a/src/components/PDFViewer.tsx b/src/components/PDFViewer.tsx
index 419630b..d4d5753 100644
--- a/src/components/PDFViewer.tsx
+++ b/src/components/PDFViewer.tsx
@@ -62,15 +62,15 @@ export function PDFViewer({ zoomLevel }: PDFViewerProps) {
   const lastDataRef = useRef<ArrayBuffer | undefined>(undefined);
 
   // Reset ready state when document data changes
-  if (currDocData !== lastDataRef.current) {
-    lastDataRef.current = currDocData;
-    if (currDocData) {
-      documentKeyRef.current += 1;
-    }
-    if (isDocumentReady) {
+  useEffect(() => {
+    if (currDocData !== lastDataRef.current) {
+      lastDataRef.current = currDocData;
+      if (currDocData) {
+        documentKeyRef.current += 1;
+      }
       setIsDocumentReady(false);
     }
-  }
+  }, [currDocData]);
 
   // Create a Uint8Array copy to prevent "detached ArrayBuffer" errors
   const pdfFileData = useMemo(() => {
@@ -300,8 +300,11 @@ export function PDFViewer({ zoomLevel }: PDFViewerProps) {
           onDocumentLoadSuccess(pdf);
           setIsDocumentReady(true);
         }}
-        onLoadError={() => {
-          // Ignore errors from destroyed documents during navigation/reload
+        onLoadError={(error) => {
+          // Log errors in development for debugging
+          if (process.env.NODE_ENV !== 'production') {
+            console.warn('PDFViewer load error (may be from destroyed document during navigation):', error);
+          }
         }}
         onItemClick={(args: PDFOnLinkClickArgs) => {
           if (args?.pageNumber) {
diff --git a/src/components/SettingsModal.tsx b/src/components/SettingsModal.tsx
index a6ca39b..c414030 100644
--- a/src/components/SettingsModal.tsx
+++ b/src/components/SettingsModal.tsx
@@ -730,15 +730,19 @@ export function SettingsModal() {
                               switch (provider.id) {
                                 case 'openai':
                                   setLocalSummaryModel('gpt-4o-mini');
+                                  setLocalSummaryBaseUrl(''); // Clear custom base URL
                                   break;
                                 case 'anthropic':
                                   setLocalSummaryModel('claude-3-5-haiku-latest');
+                                  setLocalSummaryBaseUrl(''); // Clear custom base URL
                                   break;
                                 case 'groq':
                                   setLocalSummaryModel('llama-3.3-70b-versatile');
+                                  setLocalSummaryBaseUrl(''); // Clear custom base URL
                                   break;
                                 case 'openrouter':
                                   setLocalSummaryModel('google/gemini-2.0-flash-001');
+                                  setLocalSummaryBaseUrl(''); // Clear custom base URL
                                   break;
                                 case 'custom-openai':
                                   setLocalSummaryModel('');
diff --git a/src/components/SummarizeButton.tsx b/src/components/SummarizeButton.tsx
index f9a8659..fbfc889 100644
--- a/src/components/SummarizeButton.tsx
+++ b/src/components/SummarizeButton.tsx
@@ -10,6 +10,7 @@ interface SummarizeButtonProps {
 export function SummarizeButton({ onClick, disabled }: SummarizeButtonProps) {
   return (
     <button
+      type="button"
       onClick={onClick}
       disabled={disabled}
       className="inline-flex items-center py-1 px-2 rounded-md border border-offbase bg-base text-foreground text-xs hover:bg-offbase transition-all duration-200 ease-in-out hover:scale-[1.09] hover:text-accent disabled:opacity-50 disabled:cursor-not-allowed"
diff --git a/src/lib/concurrency.ts b/src/lib/concurrency.ts
new file mode 100644
index 0000000..3267dc1
--- /dev/null
+++ b/src/lib/concurrency.ts
@@ -0,0 +1,50 @@
+/**
+ * Utility for processing items with a concurrency limit
+ *
+ * This enables parallel processing of items (like TTS generation) while
+ * respecting rate limits and preventing resource exhaustion.
+ */
+
+export type ConcurrencyResult<R> =
+  | { status: 'fulfilled'; value: R }
+  | { status: 'rejected'; reason: Error };
+
+/**
+ * Process items in parallel with a maximum concurrency limit.
+ *
+ * @param items - Array of items to process
+ * @param processor - Async function to process each item
+ * @param maxConcurrent - Maximum number of concurrent processors
+ * @param signal - Optional AbortSignal for cancellation
+ * @returns Array of results in the same order as input items
+ */
+export async function processWithConcurrencyLimit<T, R>(
+  items: T[],
+  processor: (item: T, index: number) => Promise<R>,
+  maxConcurrent: number,
+  signal?: AbortSignal
+): Promise<ConcurrencyResult<R>[]> {
+  const results: ConcurrencyResult<R>[] = new Array(items.length);
+  let currentIndex = 0;
+
+  async function worker(): Promise<void> {
+    while (currentIndex < items.length) {
+      if (signal?.aborted) break;
+
+      const index = currentIndex++;
+      if (index >= items.length) break;
+
+      try {
+        const result = await processor(items[index], index);
+        results[index] = { status: 'fulfilled', value: result };
+      } catch (error) {
+        results[index] = { status: 'rejected', reason: error as Error };
+      }
+    }
+  }
+
+  const workerCount = Math.min(maxConcurrent, items.length);
+  await Promise.all(Array(workerCount).fill(null).map(() => worker()));
+
+  return results;
+}
diff --git a/src/lib/dexie.ts b/src/lib/dexie.ts
index 74ae7d5..dae97ed 100644
--- a/src/lib/dexie.ts
+++ b/src/lib/dexie.ts
@@ -329,6 +329,7 @@ async function applyDocumentIdMapping(oldId: string, newId: string): Promise<voi
         db[LAST_LOCATION_TABLE],
         db[APP_CONFIG_TABLE],
         db[DOCUMENT_ID_MAP_TABLE],
+        db[SUMMARIES_TABLE],
       ],
       async () => {
         await recordDocumentIdMapping(oldId, nextId);
@@ -406,6 +407,24 @@ async function applyDocumentIdMapping(oldId: string, newId: string): Promise<voi
             await db[APP_CONFIG_TABLE].update('singleton', { documentListState: mapped });
           }
         }
+
+        // Remap summaries to use the new document ID
+        const oldSummaries = await db[SUMMARIES_TABLE].where('docId').equals(oldId).toArray();
+        for (const summary of oldSummaries) {
+          const newSummaryId = `${nextId}-${summary.scope}-${summary.pageNumber ?? 'all'}`;
+          // Check if a summary with the new ID already exists
+          const existing = await db[SUMMARIES_TABLE].get(newSummaryId);
+          if (!existing) {
+            // Create new summary with updated docId and id
+            await db[SUMMARIES_TABLE].put({
+              ...summary,
+              id: newSummaryId,
+              docId: nextId,
+            });
+          }
+          // Delete the old summary
+          await db[SUMMARIES_TABLE].delete(summary.id);
+        }
       },
     );
   });
diff --git a/src/lib/summarize.ts b/src/lib/summarize.ts
index a8c14f2..78d000a 100644
--- a/src/lib/summarize.ts
+++ b/src/lib/summarize.ts
@@ -24,15 +24,48 @@ function splitTextIntoChunks(text: string, maxTokens: number): string[] {
   const chunks: string[] = [];
   let current = '';
 
+  // Helper to split oversized text into multiple chunks
+  function splitOversizedText(oversized: string): string[] {
+    const parts: string[] = [];
+    let remaining = oversized;
+    while (remaining.length > 0) {
+      // Try to split at sentence boundary within maxChars
+      let splitPoint = maxChars;
+      if (remaining.length > maxChars) {
+        const searchRange = remaining.slice(0, maxChars);
+        const lastSentence = Math.max(
+          searchRange.lastIndexOf('. '),
+          searchRange.lastIndexOf('! '),
+          searchRange.lastIndexOf('? ')
+        );
+        if (lastSentence > maxChars * 0.5) {
+          splitPoint = lastSentence + 2; // Include the punctuation and space
+        }
+      }
+      parts.push(remaining.slice(0, splitPoint).trim());
+      remaining = remaining.slice(splitPoint).trim();
+    }
+    return parts;
+  }
+
   for (const para of text.split(/\n\n+/)) {
     const trimmed = para.trim();
     if (!trimmed) continue;
 
     if ((current + '\n\n' + trimmed).length > maxChars) {
       if (current) chunks.push(current.trim());
-      current = trimmed.length > maxChars 
-        ? trimmed.slice(0, maxChars) // Force split if paragraph too long
-        : trimmed;
+      // Handle oversized paragraphs by splitting them iteratively
+      if (trimmed.length > maxChars) {
+        const splitParts = splitOversizedText(trimmed);
+        // Add all but the last part as separate chunks
+        for (let i = 0; i < splitParts.length - 1; i++) {
+          chunks.push(splitParts[i]);
+        }
+        // Use the last part as the new current
+        current = splitParts[splitParts.length - 1] || '';
+      } else {
+        current = trimmed;
+      }
     } else {
       current = current ? current + '\n\n' + trimmed : trimmed;
     }
@@ -101,9 +134,19 @@ export async function generateSummary(
 
   // Combine summaries (recursively if still too large)
   onProgress?.({ currentChunk: chunks.length, totalChunks: chunks.length, phase: 'combining', message: 'Combining summaries...' });
-  
+
   let combined = summaries.join('\n\n---\n\n');
+  const MAX_COMBINING_ITERATIONS = 10;
+  let combiningIterations = 0;
+
   while (needsChunking(combined, contextLimit)) {
+    combiningIterations++;
+    if (combiningIterations > MAX_COMBINING_ITERATIONS) {
+      console.warn('Summary combining reached maximum iterations, proceeding with current result');
+      break;
+    }
+
+    const previousLength = combined.length;
     const subChunks = splitTextIntoChunks(combined, maxInputTokens);
     const subSummaries: string[] = [];
     for (const chunk of subChunks) {
@@ -112,6 +155,12 @@ export async function generateSummary(
       totalTokens += result.tokensUsed || 0;
     }
     combined = subSummaries.join('\n\n---\n\n');
+
+    // Guard against non-convergence: if combined didn't shrink, break to avoid infinite loop
+    if (combined.length >= previousLength) {
+      console.warn('Summary combining not converging, proceeding with current result');
+      break;
+    }
   }
 
   // Final pass