Update data viewer with cleaner domain architecture to prepare File Obejcts#8292
Update data viewer with cleaner domain architecture to prepare File Obejcts#8292
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThis pull request refactors the artifact file handling system to improve content type management and file operations. Changes include introducing new utility functions for file type detection and icon mapping, restructuring the data viewer to handle multiple content types through explicit branches rather than configuration maps, adding support for binary file parsing, implementing file download functionality with URL safety validation, and expanding the artifact file component with copy and download button actions. Query key generation, API parameter handling, and type definitions are updated to propagate content type information throughout the data flow. 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
pa-lem
force-pushed
the
ple-file-viewer-extraction
branch
from
b9b5e44 to
df9c1f3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/app/src/shared/components/file/api/get-file-content-from-api.ts`:
- Around line 1-11: The read function creates a new TextDecoder per chunk and
uses recursion, which can corrupt multi-byte UTF-8 characters across chunk
boundaries and risk stack overflows; fix it by refactoring read(reader:
ReadableStreamDefaultReader<Uint8Array>) to use a single TextDecoder instance
reused for all chunks and an iterative loop (while true) that repeatedly awaits
reader.read(), decodes each result.value with decoder.decode(value, { stream:
true }), appends to a buffer, and when result.done is true call
decoder.decode(undefined) or decoder.decode(new Uint8Array(), { stream: false })
to flush any remainder before returning the full string; keep the reader usage
and return type the same and ensure you handle possible undefined result.value
safely.
🧹 Nitpick comments (2)
frontend/app/src/entities/artifacts/ui/artifact-details.tsx (1)
58-64: Consider providingdownloadUrlfor download functionality.The
FileViewercomponent accepts an optionaldownloadUrlprop, but it's not provided here. Based on theFileViewerimplementation (from relevant snippets), theEmbedViewerandFileViewerFallbackcomponents usedownloadUrlfor download buttons. Without it, users may not have a download option for images, PDFs, or unsupported file types.If
urlcan serve as the download URL, consider passing it explicitly:♻️ Proposed change
<FileViewer url={CONFIG.ARTIFACTS_CONTENT_URL(artifact.storage_id.value)} + downloadUrl={CONFIG.ARTIFACTS_CONTENT_URL(artifact.storage_id.value)} fileName={`${artifactId}.${CONTENT_TYPE_CONFIG[artifact.content_type.value]?.extension ?? "txt"}`} contentType={artifact.content_type.value} />frontend/app/src/shared/components/file/ui/utils.ts (1)
18-28: Consider extracting text content types to a shared constant.
TEXT_CONTENT_TYPESduplicates the values fromDataViewerContentType. If a new text type is added toDataViewerContentType, it must also be added here manually. Consider defining a shared array constant that both modules reference, or deriving the type from the array.♻️ Optional: derive type from array
In
frontend/app/src/shared/components/data-viewer/types.ts:export const DATA_VIEWER_CONTENT_TYPES = [ "application/json", "application/yaml", "application/hcl", "application/graphql", "image/svg+xml", "text/plain", "text/markdown", "application/xml", "text/csv", ] as const; export type DataViewerContentType = (typeof DATA_VIEWER_CONTENT_TYPES)[number];Then in
utils.ts:-const TEXT_CONTENT_TYPES: DataViewerContentType[] = [ - "application/json", - // ... -]; +import { DATA_VIEWER_CONTENT_TYPES } from "@/shared/components/data-viewer/types"; + +const TEXT_CONTENT_TYPES = DATA_VIEWER_CONTENT_TYPES;
frontend/app/src/shared/components/file/api/get-file-content-from-api.ts
Outdated
Show resolved
Hide resolved
Introduces a new FileViewer component that can display various file types: - Text files (JSON, YAML, CSV, etc.) with syntax highlighting - Images (PNG, JPEG, SVG) - PDFs via embedded iframe - Fallback for unsupported types This refactors the artifact details view to use the new FileViewer instead of the old ArtifactFile component, which is now removed. Components added: - shared/components/file/ui/file-viewer.tsx - Main viewer component - shared/components/file/ui/text-file-viewer.tsx - Text content viewer - shared/components/file/ui/embed-viewer.tsx - Container for embedded content - shared/components/file/ui/file-viewer-fallback.tsx - Fallback for unsupported types - shared/components/file/ui/file-info-card.tsx - File metadata display - shared/components/file/api/ - API layer for fetching file content - shared/components/file/domain/ - React Query hooks - shared/components/data-viewer/data-viewer-*-button.tsx - Reusable action buttons - shared/utils/file.ts - File icon utilities Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
pa-lem
force-pushed
the
ple-file-viewer-extraction
branch
from
df9c1f3 to
aeeb80e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/app/src/shared/components/data-viewer/use-get-file-content.ts`:
- Around line 3-13: The fetchFileContent function swallows all failures by
returning null inside its try-catch, preventing React Query (useQuery) from
populating error state that FileViewer relies on; change fetchFileContent to let
errors propagate (or explicitly throw) instead of returning null: if fetch
throws or response.ok is false, throw an Error containing status and URL info so
useQuery.error is set and FileViewer's error check at lines ~43-44 works as
intended. Ensure the function signature still returns Promise<string | null>
only if you intentionally want null for a specific, handled case (e.g.,
204/empty), otherwise remove the catch or rethrow in the catch block.
🧹 Nitpick comments (1)
frontend/app/src/shared/components/data-viewer/types.ts (1)
29-39: Consider derivingTEXT_CONTENT_TYPESfromDataViewerContentTypeto prevent drift.The array duplicates all values from the
DataViewerContentTypeunion. If a new type is added to the union but not to this array,getViewerTypewill incorrectly classify it as unsupported.You could derive this array from the type or use a const object pattern to ensure they stay in sync.
♻️ Proposed refactor using a const object pattern
-export type DataViewerContentType = - | "application/json" - | "application/yaml" - | "application/hcl" - | "application/graphql" - | "image/svg+xml" - | "text/plain" - | "text/markdown" - | "application/xml" - | "text/csv"; +const DATA_VIEWER_CONTENT_TYPES = [ + "application/json", + "application/yaml", + "application/hcl", + "application/graphql", + "image/svg+xml", + "text/plain", + "text/markdown", + "application/xml", + "text/csv", +] as const; + +export type DataViewerContentType = (typeof DATA_VIEWER_CONTENT_TYPES)[number]; -const TEXT_CONTENT_TYPES: DataViewerContentType[] = [ - "application/json", - "application/yaml", - "application/hcl", - "application/graphql", - "image/svg+xml", - "text/plain", - "text/markdown", - "application/xml", - "text/csv", -]; +const TEXT_CONTENT_TYPES: readonly DataViewerContentType[] = DATA_VIEWER_CONTENT_TYPES;
frontend/app/src/shared/components/data-viewer/use-get-file-content.ts
Outdated
Show resolved
Hide resolved
pa-lem
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@frontend/app/src/entities/artifacts/ui/artifact-details.tsx`:
- Around line 43-45: The code currently computes extension using
TEXT_CONTENT_TYPE_CONFIG[contentType as TextContentType] and falls back to
"txt", which wrongly assigns .txt for non-text artifacts; update the logic in
artifact-details.tsx (look for variables contentType and extension and the
TEXT_CONTENT_TYPE_CONFIG lookup) to detect non-text MIME types (e.g., image/*,
application/pdf, video/*, audio/*) and choose appropriate extensions (e.g.,
derive from the MIME subtype or map common types) instead of defaulting to
"txt", and only use the TEXT_CONTENT_TYPE_CONFIG fallback for actual text
content types.
In `@frontend/app/src/shared/components/data-viewer/types.ts`:
- Around line 41-70: getViewerType currently matches contentType verbatim, so
values with parameters (e.g., "application/json; charset=utf-8") fall through to
unsupported; normalize the input by trimming and splitting on ";" (take the
media type part) and lowercasing it before any checks. Update getViewerType to
compute a normalizedContentType (string) and use that in comparisons for
TEXT_CONTENT_TYPES, the "application/x-yaml" match, startsWith("text/") and
startsWith("image/"), while preserving casts to TextContentType where
appropriate.
In `@frontend/app/src/shared/utils/file.ts`:
- Around line 33-43: The isBinaryContentType function should normalize the input
MIME before checks: trim whitespace, split on ';' and take the media type, and
lowercase it so parameterized values like "application/pdf; charset=binary" are
detected; update isBinaryContentType to compute a normalized variable (e.g.,
mediaType) from contentType and use mediaType in the existing image/ and
application/pdf comparisons (also ensure the SVG check compares against
"image/svg+xml" using the normalized mediaType).
🧹 Nitpick comments (3)
frontend/app/src/shared/components/download.tsx (1)
48-54: Revoke blob URLs to avoid memory leaks.
URL.createObjectURLshould be paired withURL.revokeObjectURLwhen the component unmounts or the blob changes.♻️ Suggested refactor
-import { Link, type LinkProps } from "react-aria-components"; +import { useEffect, useMemo } from "react"; +import { Link, type LinkProps } from "react-aria-components"; @@ export function Download({ contentType = "text/plain", value, fileName, downloadUrl, ...props }: DownloadProps) { + const shouldUseBlob = !downloadUrl; + const blobUrl = useMemo(() => { + if (!shouldUseBlob) return ""; + const blob = new Blob([value], { type: contentType }); + return URL.createObjectURL(blob); + }, [shouldUseBlob, value, contentType]); + + useEffect(() => { + if (!shouldUseBlob || !blobUrl) return; + return () => URL.revokeObjectURL(blobUrl); + }, [shouldUseBlob, blobUrl]); + // When a download URL is provided, validate and use it directly if (downloadUrl) { @@ - const blob = new Blob([value], { type: contentType }); - const url = URL.createObjectURL(blob); - - return ( - <Link href={url} target="_blank" rel="noopener noreferrer" download={fileName} {...props} /> - ); + return ( + <Link + href={blobUrl} + target="_blank" + rel="noopener noreferrer" + download={fileName} + {...props} + /> + ); }frontend/app/src/shared/utils/file.ts (1)
52-55: Consider chunked base64 conversion for large buffers.
Array.from(...).join("")can be memory-heavy for large payloads.♻️ Suggested refactor
export function arrayBufferToBase64(buffer: ArrayBuffer): string { const bytes = new Uint8Array(buffer); - const binary = Array.from(bytes, (byte) => String.fromCharCode(byte)).join(""); - return btoa(binary); + let binary = ""; + const chunkSize = 0x8000; + for (let i = 0; i < bytes.length; i += chunkSize) { + binary += String.fromCharCode(...bytes.subarray(i, i + chunkSize)); + } + return btoa(binary); }frontend/app/src/entities/artifacts/ui/artifact-file.tsx (1)
34-46: Avoid duplicate URL construction for raw downloads.
You already importgetArtifactFileDownloadUrl; use it forrawUrltoo so the URL logic stays centralized.♻️ Suggested tweak
- const rawUrl = CONFIG.ARTIFACTS_CONTENT_URL(storageId); + const rawUrl = getArtifactFileDownloadUrl(storageId);
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/app/src/entities/artifacts/ui/artifact-details.tsx`:
- Around line 43-46: The fallback when resolving extension uses the whole config
object instead of its extension field, causing fileName to become "[object
Object]"; update the computation in the artifact-details logic that sets
extension (currently using contentType, TEXT_CONTENT_TYPE_CONFIG and
TextContentType) so the fallback picks the extension string (e.g.,
TEXT_CONTENT_TYPE_CONFIG["text/plain"].extension or a literal like "txt") rather
than the config object—ensure the expression reads something like
TEXT_CONTENT_TYPE_CONFIG[contentType as TextContentType]?.extension ??
TEXT_CONTENT_TYPE_CONFIG["text/plain"].extension (or an explicit string) so
extension is always a string.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/app/src/shared/components/data-viewer/types.ts`:
- Around line 42-44: The current default branch uses contentType being falsy to
return { type: "text", textContentType: "text/plain" } which can corrupt binary
data; update the fallback in the function that checks contentType to return a
safer sentinel like { type: "unsupported" } (or alternatively add explicit
docs/comments that missing contentType is intentionally treated as text) instead
of defaulting to "text/plain", and adjust any consumers of this function to
handle the new "unsupported" type; locate the occurrence by searching for the
contentType check and the return { type: "text", textContentType: "text/plain" }
in types.ts.
🧹 Nitpick comments (1)
frontend/app/src/shared/components/data-viewer/types.ts (1)
5-14: Consider deriving the array from a const assertion to prevent type/value drift.The
TextContentTypeunion andTEXT_CONTENT_TYPESarray list the same values. If one is updated without the other, they'll silently diverge.♻️ Proposed refactor using const assertion
-export type TextContentType = - | "application/json" - | "application/yaml" - | "application/hcl" - | "application/graphql" - | "image/svg+xml" - | "text/plain" - | "text/markdown" - | "application/xml" - | "text/csv"; - -// ... - -const TEXT_CONTENT_TYPES: TextContentType[] = [ - "application/json", - "application/yaml", - "application/hcl", - "application/graphql", - "image/svg+xml", - "text/plain", - "text/markdown", - "application/xml", - "text/csv", -]; +export const TEXT_CONTENT_TYPES = [ + "application/json", + "application/yaml", + "application/hcl", + "application/graphql", + "image/svg+xml", + "text/plain", + "text/markdown", + "application/xml", + "text/csv", +] as const; + +export type TextContentType = (typeof TEXT_CONTENT_TYPES)[number];Also applies to: 29-39
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@frontend/app/src/entities/artifacts/ui/artifact-file.tsx`:
- Line 39: Remove the unused downloadUrl prop being passed into DataViewer: open
the component usage where DataViewer is rendered (the JSX that includes
downloadUrl={downloadUrl}) and delete that prop so it isn't passed; verify
DataViewerProps does not declare downloadUrl and ensure the existing actions
prop continues to use downloadUrl for download behavior (no changes to
DataViewer or actions required).
- Around line 46-47: The copy and download buttons misuse base64 binary content:
change DataViewerDownloadButton to use downloadUrl for non-text contentTypes or
decode the base64 into binary before creating a Blob (identify by props:
value/content, contentType, fileName, downloadUrl) so downloads contain real
binary data instead of base64 text; and update DataViewerCopyButton to be
contentType-aware (hide or disable the copy action for non-text types like
images/pdf) so the clipboard is not populated with raw base64 for binary files.
🧹 Nitpick comments (2)
frontend/app/src/shared/components/data-viewer/data-viewer.tsx (2)
56-76: Image and PDF rendering assumes base64-encoded content.The rendering logic constructs data URLs assuming
contentis already base64-encoded. This implicit contract should be documented or validated. If raw binary data is passed, rendering will fail silently.Consider adding a brief comment clarifying the expected format, or validate the base64 encoding.
117-133:getLanguageduplicates logic fromgetExtensionFromContentType.Both functions map content types to similar strings (extensions vs language identifiers). Consider consolidating or creating a shared mapping to reduce maintenance burden.
💡 Possible consolidation approach
Create a shared content-type config in
types.ts:const CONTENT_TYPE_CONFIG: Record<TextContentType, { extension: string; language: string }> = { "application/json": { extension: "json", language: "json" }, "application/yaml": { extension: "yaml", language: "yaml" }, // ... etc };Then both
getExtensionFromContentTypeandgetLanguagecan reference this single source of truth.
pa-lem
changed the title
frontend/app/src/entities/graphql/ui/graphql-query-viewer-card.tsx
Outdated
Show resolved
Hide resolved
2 participants
Summary
ArtifactFilecomponent that uses the existing domain layer for fetching artifact contentDataViewerto handle all content types (text, images, PDFs, unsupported)FileViewerand related utilities that bypassed the domain layerSummary by CodeRabbit
Release Notes
New Features
Improvements