refactor preview.tsx to make it more manageable #2351
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughRefactors the preview feature into a modular architecture. The PreviewModel class was moved from frontend/app/view/preview/preview.tsx to a new frontend/app/view/preview/preview-model.tsx and now centralizes file state, RPC I/O, navigation, edit/save/revert workflows, and specialized-view selection. Several new specialized view components were added: preview-edit.tsx (code editor), preview-markdown.tsx (Markdown), preview-streaming.tsx (PDF/video/audio/image), and preview-directory.tsx was updated. preview.tsx was simplified to compose these specialized components and now exports SpecializedViewProps. Minor import updates were applied (e.g., block.tsx, preview-directory.tsx). Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
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: 0
🧹 Nitpick comments (18)
frontend/app/view/preview/preview-directory.tsx (4)
80-97: Fix unit conversion edge cases (>= threshold and allow highest unit).1024 bytes shows as “1024b” instead of “1k”, and values never reach “t”. Update the loop to use >= and compute the unit after dividing.
Apply:
-function getBestUnit(bytes: number, si: boolean = false, sigfig: number = 3): string { +function getBestUnit(bytes: number, si: boolean = false, sigfig: number = 3): string { if (bytes === undefined || bytes < 0) { return "-"; } const units = si ? ["kB", "MB", "GB", "TB"] : ["KiB", "MiB", "GiB", "TiB"]; const divisor = si ? 1000 : 1024; - let currentUnit = "B"; - let currentValue = bytes; - let idx = 0; - while (currentValue > divisor && idx < units.length - 1) { - currentUnit = units[idx]; - currentValue /= divisor; - idx += 1; - } + let currentValue = bytes; + let idx = 0; + while (currentValue >= divisor && idx < units.length) { + currentValue /= divisor; + idx += 1; + } + const currentUnit = idx === 0 ? "B" : units[idx - 1]; return `${parseFloat(currentValue.toPrecision(sigfig))}${displaySuffixes[currentUnit]}`; }
215-227: Refine MIME fallback: avoid character‑by‑character truncation.Slicing one char at a time can miss useful fallbacks (“text/markdown” → “text/*”/“text”). Prefer hierarchical fallback by major type.
Example:
-const getIconFromMimeType = useCallback( - (mimeType: string): string => { - while (mimeType.length > 0) { - const icon = fullConfig.mimetypes?.[mimeType]?.icon ?? null; - if (isIconValid(icon)) { - return `fa fa-solid fa-${icon} fa-fw`; - } - mimeType = mimeType.slice(0, -1); - } - return "fa fa-solid fa-file fa-fw"; - }, - [fullConfig.mimetypes] -); +const getIconFromMimeType = useCallback( + (mimeType: string): string => { + const clean = cleanMimetype(mimeType ?? ""); + const [major] = clean.split("/"); + const candidates = [clean, `${major}/*`, major]; + for (const key of candidates) { + const icon = fullConfig.mimetypes?.[key]?.icon ?? null; + if (isIconValid(icon)) return `fa fa-solid fa-${icon} fa-fw`; + } + return "fa fa-solid fa-file fa-fw"; + }, + [fullConfig.mimetypes] +);
526-556: Guard against transient nulls from OverlayScrollbars and simplify scrolling.Minor stability: use optional chaining to avoid rare NPEs during mount/unmount race. Optionally, use Element.scrollIntoView for clarity.
-const viewport = osRef.osInstance().elements().viewport; +const viewport = osRef?.osInstance?.()?.elements?.().viewport; if (!viewport) return;Or:
-viewport.scrollTo({ top: topVal }); +viewport.scrollTo?.({ top: topVal });
669-693: Use stable row keys to reduce remounts during resort/filter.Index keys can cause unnecessary remounts. Prefer row.id.
- {table.getTopRows().map((row, idx) => ( + {table.getTopRows().map((row, idx) => ( <TableRow ... - key={idx} + key={row.id} /> ))} - {table.getCenterRows().map((row, idx) => ( + {table.getCenterRows().map((row, idx) => ( <TableRow ... - key={idx} + key={row.id} /> ))}frontend/app/view/preview/preview-markdown.tsx (1)
10-21: Null‑safe resolve options and avoid relying on global types.Accessing fileInfo.dir can throw if stat hasn’t loaded. Also, remove explicit MarkdownResolveOpts if not imported.
Apply:
-function MarkdownPreview({ model }: SpecializedViewProps) { +function MarkdownPreview({ model }: SpecializedViewProps) { const connName = useAtomValue(model.connection); const fileInfo = useAtomValue(model.statFile); const fontSizeOverride = useAtomValue(getOverrideConfigAtom(model.blockId, "markdown:fontsize")); const fixedFontSizeOverride = useAtomValue(getOverrideConfigAtom(model.blockId, "markdown:fixedfontsize")); - const resolveOpts: MarkdownResolveOpts = useMemo<MarkdownResolveOpts>(() => { - return { - connName: connName, - baseDir: fileInfo.dir, - }; - }, [connName, fileInfo.dir]); + const baseDir = fileInfo?.dir ?? ""; + const resolveOpts = useMemo(() => ({ connName, baseDir }), [connName, baseDir]);frontend/app/view/preview/preview-edit.tsx (2)
46-64: onMount must return a cleanup function; returning null breaks the declared type.Return a no‑op disposer to satisfy CodeEditor’s expected signature.
- function onMount(editor: MonacoTypes.editor.IStandaloneCodeEditor, monaco: Monaco): () => void { + function onMount(editor: MonacoTypes.editor.IStandaloneCodeEditor, monaco: Monaco): () => void { model.monacoRef.current = editor; ... - return null; + return () => { /* no-op cleanup; editor disposes on unmount */ }; }
38-45: Consider adding deps to useEffect for handler registration.If model instance ever changes, handlers may become stale. Low risk if model is stable per block.
- }, []); + }, [model]);frontend/app/view/preview/preview-streaming.tsx (3)
12-28: Typo: Rename ImageZooomControls → ImageZoomControls.Minor polish; update references.
-function ImageZooomControls() { +function ImageZoomControls() { const { zoomIn, zoomOut, resetTransform } = useControls(); ... } ... - <ImageZooomControls /> + <ImageZoomControls />Also applies to: 36-37
47-58: Add null‑safety before using fileInfo and pass alt text to images.Prevents crashes while loading, and improves a11y.
function StreamingPreview({ model }: SpecializedViewProps) { - const conn = useAtomValue(model.connection); - const fileInfo = useAtomValue(model.statFile); - const filePath = fileInfo.path; + const conn = useAtomValue(model.connection); + const fileInfo = useAtomValue(model.statFile); + if (!fileInfo) return <CenteredDiv>Loading…</CenteredDiv>; + const filePath = fileInfo.path; ... - if (fileInfo.mimetype.startsWith("image/")) { - return <StreamingImagePreview url={streamingUrl} />; + if (fileInfo.mimetype.startsWith("image/")) { + return <StreamingImagePreview url={streamingUrl} alt={fileInfo.name ?? "image"} />; }And update the image component:
-function StreamingImagePreview({ url }: { url: string }) { +function StreamingImagePreview({ url, alt }: { url: string; alt?: string }) { return ( <div className="view-preview view-preview-image"> <TransformWrapper initialScale={1} centerOnInit pinch={{ step: 10 }}> <> - <ImageZooomControls /> + <ImageZoomControls /> <TransformComponent> - <img src={url} /> + <img src={url} alt={alt ?? ""} /> </TransformComponent> </> </TransformWrapper> </div> ); }Also applies to: 84-85
61-62: A11y and media hints: add title and MIME types.Small UX/a11y improvements for assistive tech and better media selection.
- <iframe src={streamingUrl} width="100%" height="100%" name="pdfview" /> + <iframe src={streamingUrl} width="100%" height="100%" name="pdfview" title="PDF preview" />- <video controls> - <source src={streamingUrl} /> + <video controls> + <source src={streamingUrl} type={fileInfo.mimetype} /> </video>- <audio controls> - <source src={streamingUrl} /> + <audio controls> + <source src={streamingUrl} type={fileInfo.mimetype} /> </audio>Also applies to: 68-70, 77-79
frontend/app/view/preview/preview.tsx (2)
65-65: Fix typo in error messageThere's a typo in the error message: "Specialzied" should be "Specialized".
Apply this diff to fix the typo:
- return <CenteredDiv>Invalid Specialzied View Component ({specializedView.specializedView})</CenteredDiv>; + return <CenteredDiv>Invalid Specialized View Component ({specializedView.specializedView})</CenteredDiv>;
116-122: Console.log statement should be removed or replaced with proper loggingDebug console.log statements should be removed from production code or replaced with a proper logging mechanism.
Consider removing or replacing with a proper logging utility:
- useEffect(() => { - console.log("fileInfo or connection changed", fileInfo, connection); - if (!fileInfo) { - return; - } - setErrorMsg(null); - }, [connection, fileInfo]); + useEffect(() => { + if (!fileInfo) { + return; + } + setErrorMsg(null); + }, [connection, fileInfo]);frontend/app/view/preview/preview-model.tsx (6)
505-505: Fix typo in error messageThere's a typo in the error message: "Preiview" should be "Preview".
Apply this diff to fix the typo:
- return { errorStr: "File Too Large to Preiview (10 MB Max)" }; + return { errorStr: "File Too Large to Preview (10 MB Max)" };
507-508: Fix typo in error messageAnother instance of the same typo: "Preiview" should be "Preview".
Apply this diff to fix the typo:
- return { errorStr: "CSV File Too Large to Preiview (1 MB Max)" }; + return { errorStr: "CSV File Too Large to Preview (1 MB Max)" };
630-631: Console.log statement should be removed from production codeDebug console.log statement should be removed or replaced with proper logging.
Consider removing the debug log:
- console.log("not saving file, newFileContent is null"); return;
642-642: Console.log statement should be removed from production codeAnother debug console.log that should be removed.
Consider removing the debug log:
- console.log("saved file", filePath);
35-55: Consider extracting text MIME types to a constant or configurationThe hardcoded list of text application MIME types could be moved to a configuration file or constant that can be more easily maintained and extended.
Consider extracting to a separate configuration:
// In a separate config file or at the top of the module export const TEXT_APPLICATION_MIMETYPES = new Set([ "application/sql", "application/x-php", // ... rest of the types ]); // Then use it in the function function isTextFile(mimeType: string): boolean { if (mimeType == null) { return false; } return ( mimeType.startsWith("text/") || TEXT_APPLICATION_MIMETYPES.has(mimeType) || // ... rest of the logic ); }
589-589: Inconsistent error handling for console.errorWhile console.error is more appropriate than console.log for error cases, consider using a centralized error handling/logging service for consistency.
Consider creating a logging utility that can be configured for different environments.
Also applies to: 675-675
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
frontend/app/block/block.tsx(1 hunks)frontend/app/view/preview/preview-directory.tsx(3 hunks)frontend/app/view/preview/preview-edit.tsx(1 hunks)frontend/app/view/preview/preview-markdown.tsx(1 hunks)frontend/app/view/preview/preview-model.tsx(1 hunks)frontend/app/view/preview/preview-streaming.tsx(1 hunks)frontend/app/view/preview/preview.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
frontend/app/view/preview/preview-model.tsx (4)
frontend/app/block/blocktypes.ts (1)
BlockNodeModel(7-12)pkg/wshrpc/wshrpctypes.go (2)
ConnStatus(615-626)FileInfo(412-427)pkg/vdom/vdom_types.go (1)
WaveKeyboardEvent(233-247)frontend/util/previewutil.ts (1)
addOpenMenuItems(6-77)
frontend/app/view/preview/preview-edit.tsx (3)
frontend/app/view/preview/preview.tsx (1)
SpecializedViewProps(24-27)pkg/vdom/vdom_types.go (1)
WaveKeyboardEvent(233-247)frontend/app/view/codeeditor/codeeditor.tsx (1)
CodeEditor(120-194)
frontend/app/view/preview/preview-markdown.tsx (2)
frontend/app/view/preview/preview.tsx (1)
SpecializedViewProps(24-27)frontend/app/store/global.ts (1)
getOverrideConfigAtom(779-779)
frontend/app/view/preview/preview-streaming.tsx (3)
frontend/app/view/preview/preview.tsx (1)
SpecializedViewProps(24-27)frontend/util/endpoints.ts (1)
getWebServerEndpoint(10-10)frontend/app/element/quickelems.tsx (1)
CenteredDiv(19-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Build for TestDriver.ai
🔇 Additional comments (8)
frontend/app/view/preview/preview-directory.tsx (2)
36-36: Type‑only import is appropriate for PreviewModel.Keeps bundle lean and avoids circular value imports. No change requested.
735-740: Drag ref wiring LGTM.Nice, memoized callback avoids re‑binding and safely handles null.
frontend/app/view/preview/preview-edit.tsx (1)
66-76: No action required —languageprop is optional.
CodeEditorProps declareslanguage?: stringin frontend/app/view/codeeditor/codeeditor.tsx (line 114).frontend/app/block/block.tsx (1)
13-13: Import path update looks correct — verification incomplete. Automated search in the sandbox failed ("No files were searched"); confirm locally that no files import '@/app/view/preview/preview' or reference PreviewModel (e.g. run: rg -n --hidden -S "@/app/view/preview/preview" && rg -n --hidden -S "PreviewModel").frontend/app/view/preview/preview.tsx (2)
29-35: Good modular design with specialized view componentsThe SpecializedViewMap provides a clean separation of concerns by mapping view types to their respective components. This design makes it easy to add new specialized views and improves maintainability.
24-27: Well-structured exported type for component communicationThe SpecializedViewProps type provides a clear interface for specialized view components, ensuring type safety and consistency across all preview implementations.
frontend/app/view/preview/preview-model.tsx (2)
110-159: Well-architected PreviewModel with clear separation of concernsThe PreviewModel class is well-structured with:
- Clear property organization (view state, file state, connection state)
- Proper use of atoms for reactive state management
- Good separation between UI state and data state
- Comprehensive error handling atoms
This refactoring significantly improves maintainability compared to having everything in a single file.
477-529: Excellent specialized view selection logicThe getSpecializedView method provides clear, maintainable logic for determining which view to use based on file properties. The error handling is comprehensive and provides useful feedback to users.
1 participant
no functionality changes, but splits the giant preview.tsx to a model file and the sub-views for more manageable code.