Knowledge Base: Preview functionality#192
Open
Ayush8923 wants to merge 8 commits into
Open
Conversation
📝 WalkthroughWalkthroughRefactors document preview flow with shared CSV utilities and preview source logic, adds a same-origin preview proxy route, composes a refactored preview modal and CSV preview component, consolidates types, updates apiClient auth handling, adds a TTS barrel, and moves scrollbar styling to global rules. ChangesDocument Preview System Refactoring
Sequence Diagram(s)sequenceDiagram
participant Client
participant PreviewRoute
participant UpstreamHost
Client->>PreviewRoute: GET /api/document/{id}/preview
PreviewRoute->>PreviewRoute: fetch document detail via apiClient (include_url=true)
PreviewRoute->>UpstreamHost: server-fetch signed_url (GET signed_url)
UpstreamHost-->>PreviewRoute: 200 + Content-Type + body stream
PreviewRoute-->>Client: 200 streamed body (Content-Type preserved, Cache-Control: private, max-age=300)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
app/components/text-to-speech/TTSDatasetCard.tsx (1)
5-5: ⚡ Quick winUse
@/import alias for consistency with coding guidelines.This file now uses a relative import (
./DatasetDescription) while other imports use the@/alias (lines 3-4). As per coding guidelines, the project should consistently use the@/import alias for all imports.♻️ Suggested fix for consistency
-import TTSDatasetDescription from "./DatasetDescription"; +import { DatasetDescription as TTSDatasetDescription } from "`@/app/components/text-to-speech`";Or if not using the barrel export:
-import TTSDatasetDescription from "./DatasetDescription"; +import TTSDatasetDescription from "`@/app/components/text-to-speech/DatasetDescription`";As per coding guidelines: Use the import alias
@/to import from the project root as configured in tsconfig.json.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/components/text-to-speech/TTSDatasetCard.tsx` at line 5, Replace the relative import of the DatasetDescription module with the project root alias import to follow the coding guidelines: update the import that brings in TTSDatasetDescription (currently `./DatasetDescription`) to use the `@/` alias variant that other files use and ensure the imported symbol name remains TTSDatasetDescription; no other code changes needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/api/document/`[document_id]/preview/route.ts:
- Line 46: The fetch to signedUrl (const upstream = await fetch(signedUrl)) has
no timeout; wrap the request with an AbortController and a bounded timer (e.g.,
UPSTREAM_FETCH_TIMEOUT_MS) so you pass controller.signal into fetch and call
controller.abort() when the timer elapses, clear the timeout on success or
failure to avoid leaks, and ensure you handle AbortError when awaiting upstream
to return a 504/appropriate error from the route handler.
- Around line 33-44: The route currently ignores the apiClient response status
and treats any non-success as "Document has no signed URL"; update the logic
around apiClient(...) and the DocumentDetailEnvelope handling to first check the
response status and propagate it (returning NextResponse with that status) and
extract the backend error message using the standard extractor (body.error ||
body.message || body.detail or extractErrorMessage(body, fallback)) before you
access signed_url; only if the call succeeded and signedUrl
(detail.data?.signed_url || detail.signed_url) is missing should you return the
404 "Document has no signed URL" error.
In `@app/components/knowledge-base/DocumentPreviewModal.tsx`:
- Around line 52-64: The iframe timeout state persists across modal reopens
because the useEffect that manages setFrameTimedOut/setIsFrameLoading doesn't
depend on the modal open state; update the effect around useEffect to include
the open prop in the dependency array and gate the timeout logic with a check
for open (skip timeout work while closed). On open becoming false immediately
reset setFrameTimedOut(false) and setIsFrameLoading(false); when open is true
run the existing logic (checking usesIframe and previewUrl, starting the timer
using IFRAME_TIMEOUT_MS[kind], and clearing it in the cleanup). Ensure you
reference the same symbols: useEffect, open, previewDoc?.id, previewUrl,
usesIframe, kind, IFRAME_TIMEOUT_MS, setFrameTimedOut, setIsFrameLoading.
In `@app/lib/utils/csv.ts`:
- Around line 31-34: The parseCsv function incorrectly splits input on /\r?\n/
before parsing, breaking quoted cells that contain embedded newlines; update
parseCsv to split into records using a quote-aware algorithm instead of a plain
newline split (e.g., iterate through the input and accumulate characters/lines
until parseCsvRow would see balanced quotes or use a regex that matches CSV
records not inside quotes) and then call parseCsvRow for each complete record;
ensure you still derive headers from the first complete record and produce rows
by mapping parseCsvRow over remaining complete records (refer to parseCsv and
parseCsvRow to locate the logic to change).
In `@app/lib/utils/documentPreview.ts`:
- Around line 81-93: The current preview logic in documentPreview.ts returns
external viewer URLs that include doc.signed_url (seen where OFFICE_EXTS and
GOOGLE_EXTS are handled), leaking time-bound signed URLs to third parties;
change this to route previews through an internal preview proxy endpoint (or
generate a short-lived proxy token) instead of embedding doc.signed_url directly
— modify the OFFICE_EXTS/GOOGLE_EXTS branches to call your server-side preview
proxy (e.g., /internal/preview?docId=... or a proxy token) so the server fetches
the signed document and forwards it to Microsoft/Google viewers or renders it
without exposing doc.signed_url, and ensure doc.signed_url is never interpolated
into external URLs anywhere in documentPreview.ts.
---
Nitpick comments:
In `@app/components/text-to-speech/TTSDatasetCard.tsx`:
- Line 5: Replace the relative import of the DatasetDescription module with the
project root alias import to follow the coding guidelines: update the import
that brings in TTSDatasetDescription (currently `./DatasetDescription`) to use
the `@/` alias variant that other files use and ensure the imported symbol name
remains TTSDatasetDescription; no other code changes needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4363411a-9957-47e8-8d55-06d034e280c3
📒 Files selected for processing (15)
app/(main)/text-to-speech/page.tsxapp/api/document/[document_id]/preview/route.tsapp/components/analytics/BreakdownPanel.tsxapp/components/evaluations/DatasetsTab.tsxapp/components/knowledge-base/CollectionDetail.tsxapp/components/knowledge-base/CsvPreview.tsxapp/components/knowledge-base/DocumentPreviewModal.tsxapp/components/text-to-speech/DatasetsTab.tsxapp/components/text-to-speech/TTSDatasetCard.tsxapp/components/text-to-speech/index.tsapp/globals.cssapp/hooks/useSttData.tsapp/lib/types/speechToText.tsapp/lib/utils/csv.tsapp/lib/utils/documentPreview.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/lib/types/document.ts (1)
11-18: 💤 Low valueConsider adding JSDoc to clarify the envelope pattern.
The
DocumentDetailEnvelopestructure allowssigned_urlandfnameat both the top level (via extension) and nested in the optionaldatafield. This appears intentional to support API response variations, but the design may not be immediately obvious to future maintainers.Adding JSDoc comments would clarify:
- Why
DocumentDetailfields are all optional despite "Detail" suggesting more info- The fallback pattern between
data?.signed_urland top-levelsigned_url- That these types model API response shapes rather than domain entities
📝 Example JSDoc annotations
+/** + * Partial document metadata returned by the document detail API. + * Fields are optional as the API may return incomplete data. + */ export interface DocumentDetail { signed_url?: string; fname?: string; } +/** + * Document detail API response envelope. + * Supports both top-level fields and nested `data` for flexibility. + */ export interface DocumentDetailEnvelope extends DocumentDetail { data?: DocumentDetail; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/lib/types/document.ts` around lines 11 - 18, Add JSDoc comments to DocumentDetail and DocumentDetailEnvelope to document the envelope pattern: explain that DocumentDetail's fields (signed_url, fname) are optional because these types model API response shapes (not domain entities), describe that DocumentDetailEnvelope extends DocumentDetail and may also carry a nested data?: DocumentDetail as a fallback, and state the intended precedence (prefer data?.signed_url/fname if present, otherwise use top-level signed_url/fname) so future maintainers understand the design and lookup behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/lib/apiClient.ts`:
- Around line 276-286: The current early-return logic in apiFetchResponse
incorrectly returns for all non-401 responses, preventing detection of revoked
sessions; change the flow so you return immediately only when res.ok, but for
non-ok responses check for a revoked-session signal (e.g., if res.status === 403
then read/parsethe response body/text and detect the string "revoked") and call
dispatchAuthExpired() when found; keep the existing guard for auth endpoints
(url.startsWith("/api/auth/")) to avoid auto-refresh, continue to use
tryRefreshToken() and doFetch() for 401 handling, and ensure you still return
the original response after dispatching AUTH_EXPIRED.
---
Nitpick comments:
In `@app/lib/types/document.ts`:
- Around line 11-18: Add JSDoc comments to DocumentDetail and
DocumentDetailEnvelope to document the envelope pattern: explain that
DocumentDetail's fields (signed_url, fname) are optional because these types
model API response shapes (not domain entities), describe that
DocumentDetailEnvelope extends DocumentDetail and may also carry a nested data?:
DocumentDetail as a fallback, and state the intended precedence (prefer
data?.signed_url/fname if present, otherwise use top-level signed_url/fname) so
future maintainers understand the design and lookup behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a13efe7d-1fdd-47a9-8bed-4b77f4dcedc6
📒 Files selected for processing (6)
app/api/document/[document_id]/preview/route.tsapp/components/knowledge-base/CsvPreview.tsxapp/components/knowledge-base/DocumentPreviewModal.tsxapp/lib/apiClient.tsapp/lib/types/document.tsapp/lib/utils/documentPreview.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- app/components/knowledge-base/CsvPreview.tsx
- app/api/document/[document_id]/preview/route.ts
- app/components/knowledge-base/DocumentPreviewModal.tsx
vprashrex
approved these changes
Jun 5, 2026
Co-authored-by: Prashant Vasudevan <71649489+vprashrex@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue: #184
Summary:
.docsand.txtfile, so fix the preview functionality for this and if the not able to preview then some prompt will show so that user know this file not able to preview.