refactor(file-viewer): replace react-doc-viewer with in-house renderers#95
Closed
kemuru wants to merge 2 commits into
Closed
refactor(file-viewer): replace react-doc-viewer with in-house renderers#95kemuru wants to merge 2 commits into
kemuru wants to merge 2 commits into
Conversation
✅ Deploy Preview for kleros-v2-ui-storybook ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
WalkthroughReplaces the ChangesFile Viewer Rewrite
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
src/lib/file-viewer/svg-viewer.tsx (1)
17-20: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNo transparency checkerboard, unlike
ImageViewer.Transparent SVGs will render on plain background here instead of the checkerboard
ImageVieweruses for transparent rasters, which is a minor visual inconsistency between the two renderers.🤖 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 `@src/lib/file-viewer/svg-viewer.tsx` around lines 17 - 20, The SVG viewer container currently renders transparent SVGs on a plain background, unlike ImageViewer’s checkerboard treatment. Update svg-viewer.tsx in the SVG rendering container to use the same transparency checkerboard background styling/pattern as ImageViewer so both renderers behave consistently for transparent content.src/lib/file-viewer/pdf-viewer.tsx (1)
62-63: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftAll PDF pages render simultaneously with no virtualization.
For large multi-page PDFs (common for legal/evidence documents), rendering every page unconditionally can cause heavy DOM/memory usage and a sluggish scroll experience. Consider windowing (e.g., only rendering pages near the viewport) or lazy-mounting pages as they scroll into view.
Also applies to: 95-111
🤖 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 `@src/lib/file-viewer/pdf-viewer.tsx` around lines 62 - 63, The PDF viewer currently renders every page at once in PdfViewer, which can cause heavy DOM and memory usage for large documents. Update the page rendering logic to use virtualization or lazy mounting so only pages near the viewport are rendered, and wire it through the existing PdfViewer state and page map/rendering flow that uses numPages and zoom. Prefer a windowing approach tied to scroll position or intersection visibility rather than unconditional page rendering.src/lib/file-viewer/csv-viewer.tsx (1)
14-18: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPapa.parse errors are silently ignored.
Papa.parsereturns aerrorsarray for malformed rows (e.g., inconsistent field counts), but onlyparsed.datais used. A CSV with parse errors will render whatever rows Papa managed to extract with no indication anything went wrong, which can be confusing when the displayed table looks incomplete or malformed.♻️ Surface parse errors
- const rows = useMemo<string[][]>(() => { + const [parseFailed, setParseFailed] = useState(false); + const rows = useMemo<string[][]>(() => { if (!text) return []; const parsed = Papa.parse<string[]>(text.trim(), { skipEmptyLines: true }); + if (parsed.errors.length) setParseFailed(true); return parsed.data; }, [text]);🤖 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 `@src/lib/file-viewer/csv-viewer.tsx` around lines 14 - 18, Papa.parse errors are currently ignored in the CSV viewer, so malformed input can render without any warning. Update the csv-viewer.tsx logic in the useMemo block that builds rows from Papa.parse to inspect parsed.errors as well as parsed.data, and surface a parse-failure state when errors are present. Use the existing CSV viewer flow in CsvViewer to either collect and expose the error details or fall back to an empty/error state so the UI can indicate the parse problem instead of silently showing partial rows.src/lib/file-viewer/use-file-text.ts (1)
22-43: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSolid abort-safe fetch hook. AbortController cleanup and guard checks (
controller.signal.aborted) correctly prevent stale-state updates after unmount/uri-change.One gap: there's no fetch timeout, so if the network hangs,
loadingstaystrueindefinitely and the renderer shows "Loading…" forever with no way to recover short of unmounting. Consider racing the fetch against a timeout that flipserror: true.♻️ Add a fetch timeout
useEffect(() => { const controller = new AbortController(); setState({ text: null, loading: true, error: false }); + const timeoutId = setTimeout(() => controller.abort(), 15000); fetch(uri, { signal: controller.signal }) .then((res) => { if (!res.ok) throw new Error(`HTTP ${res.status}`); return res.text(); }) .then((text) => { if (!controller.signal.aborted) { setState({ text, loading: false, error: false }); } }) .catch(() => { if (!controller.signal.aborted) { setState({ text: null, loading: false, error: true }); + } else { + setState({ text: null, loading: false, error: true }); } }); - return () => controller.abort(); + return () => { + clearTimeout(timeoutId); + controller.abort(); + }; }, [uri]);🤖 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 `@src/lib/file-viewer/use-file-text.ts` around lines 22 - 43, The abort-safe fetch logic in useFileText still has no timeout, so a hanging request can leave loading stuck forever. Update the useEffect in useFileText to race fetch against a timeout (using the existing AbortController cleanup) and, on timeout, abort the request and set the state to a recoverable error state with loading false and error true. Keep the stale-update guards based on controller.signal.aborted so uri changes and unmounts remain safe.
🤖 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 `@src/lib/file-viewer/download-button.tsx`:
- Around line 51-68: The download flow in DownloadButton still allows `fileName`
to be missing, which causes the native download attribute to be omitted and the
same-origin/blob/data branches to fall back to opening a new tab. Update the
`DownloadButton` component so it always derives a fallback name (for example via
`fileNameFromUrl(url)`) and uses that value consistently when building `href`
and when setting the `download` attribute, keeping the behavior aligned across
the `useMemo` URL handling and the button render logic.
In `@src/lib/file-viewer/image-viewer.tsx`:
- Around line 24-40: The static ids in ImageViewer are duplicated across viewer
instances and conflict with SvgViewer, so remove the hardcoded
id="image-renderer" and id="image-img" from ImageViewer or replace them with
non-DOM-unique test hooks such as data-testid. Keep the existing styling via
className/checkerboard intact, and make the same adjustment anywhere the shared
viewer pattern relies on these ids.
In `@src/lib/file-viewer/index.tsx`:
- Around line 139-140: Update the JSDoc for the file viewer component to include
video as a supported format, since FileViewer now wires VideoViewer. Keep the
existing description in sync with the actual supported types listed by the
component so the docs mention PDFs, images, SVG, markdown, plaintext, CSV, and
video.
In `@src/lib/file-viewer/pdf-viewer.tsx`:
- Around line 57-63: The initial zoom state in PdfViewer is taken directly from
defaultZoom, so it can start outside the allowed range before any user
interaction. Clamp the initial value used by the useState for zoom to MIN_ZOOM
and MAX_ZOOM, matching the behavior already enforced by changeZoom, and ensure
config.pdfDefaultZoom cannot initialize PdfViewer outside bounds.
---
Nitpick comments:
In `@src/lib/file-viewer/csv-viewer.tsx`:
- Around line 14-18: Papa.parse errors are currently ignored in the CSV viewer,
so malformed input can render without any warning. Update the csv-viewer.tsx
logic in the useMemo block that builds rows from Papa.parse to inspect
parsed.errors as well as parsed.data, and surface a parse-failure state when
errors are present. Use the existing CSV viewer flow in CsvViewer to either
collect and expose the error details or fall back to an empty/error state so the
UI can indicate the parse problem instead of silently showing partial rows.
In `@src/lib/file-viewer/pdf-viewer.tsx`:
- Around line 62-63: The PDF viewer currently renders every page at once in
PdfViewer, which can cause heavy DOM and memory usage for large documents.
Update the page rendering logic to use virtualization or lazy mounting so only
pages near the viewport are rendered, and wire it through the existing PdfViewer
state and page map/rendering flow that uses numPages and zoom. Prefer a
windowing approach tied to scroll position or intersection visibility rather
than unconditional page rendering.
In `@src/lib/file-viewer/svg-viewer.tsx`:
- Around line 17-20: The SVG viewer container currently renders transparent SVGs
on a plain background, unlike ImageViewer’s checkerboard treatment. Update
svg-viewer.tsx in the SVG rendering container to use the same transparency
checkerboard background styling/pattern as ImageViewer so both renderers behave
consistently for transparent content.
In `@src/lib/file-viewer/use-file-text.ts`:
- Around line 22-43: The abort-safe fetch logic in useFileText still has no
timeout, so a hanging request can leave loading stuck forever. Update the
useEffect in useFileText to race fetch against a timeout (using the existing
AbortController cleanup) and, on timeout, abort the request and set the state to
a recoverable error state with loading false and error true. Keep the
stale-update guards based on controller.signal.aborted so uri changes and
unmounts remain safe.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2c4eb886-b850-4f3b-9479-a34070405ab0
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (15)
package.jsonsrc/lib/file-viewer/csv-viewer.tsxsrc/lib/file-viewer/download-button.tsxsrc/lib/file-viewer/image-viewer.tsxsrc/lib/file-viewer/index.tsxsrc/lib/file-viewer/markdown-viewer.tsxsrc/lib/file-viewer/pdf-viewer.tsxsrc/lib/file-viewer/status.tsxsrc/lib/file-viewer/svg-viewer.tsxsrc/lib/file-viewer/text-viewer.tsxsrc/lib/file-viewer/types.tssrc/lib/file-viewer/use-file-text.tssrc/lib/file-viewer/use-file-type.tssrc/lib/file-viewer/video-viewer.tsxsrc/stories/file-viewer.stories.tsx
|
Contributor
Author
|
turns out it's not needed |
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
1 participant
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.
PR-Codex overview
This PR focuses on enhancing the file viewer component by adding new renderers for various file types, improving the handling of different file formats, and updating the component structure to ensure better security and usability.
Detailed summary
SvgDocRenderertoSvgViewer.VideoViewer,TextViewer, andCsvViewercomponents.ViewerMessagefor consistent loading/error messages.FileViewerto utilize new renderers and improved file type detection.DownloadButtonfunctionality for safer file downloads.package.json.Summary by CodeRabbit