Implement preview documents via click file in artifact detail#41
Implement preview documents via click file in artifact detail#41synasapmob merged 5 commits intodevfrom
Conversation
synasapmob
force-pushed
the
feat/ENG-1339
branch
5 times, most recently
from
746d77f to
ee74e11
Compare
This comment was marked as spam.
This comment was marked as spam.
5 similar comments
|
@synasapmob The model is not available for your account. This can happen if the model was disabled by your organization's policy or if your Copilot plan doesn't include access to it. You can try again without specifying a model (just If you want to contact GitHub about this error, please mention the following identifier so they can better serve you: |
|
@synasapmob The model is not available for your account. This can happen if the model was disabled by your organization's policy or if your Copilot plan doesn't include access to it. You can try again without specifying a model (just If you want to contact GitHub about this error, please mention the following identifier so they can better serve you: |
|
@synasapmob The model is not available for your account. This can happen if the model was disabled by your organization's policy or if your Copilot plan doesn't include access to it. You can try again without specifying a model (just If you want to contact GitHub about this error, please mention the following identifier so they can better serve you: |
|
@synasapmob The model is not available for your account. This can happen if the model was disabled by your organization's policy or if your Copilot plan doesn't include access to it. You can try again without specifying a model (just If you want to contact GitHub about this error, please mention the following identifier so they can better serve you: |
|
@synasapmob The model is not available for your account. This can happen if the model was disabled by your organization's policy or if your Copilot plan doesn't include access to it. You can try again without specifying a model (just If you want to contact GitHub about this error, please mention the following identifier so they can better serve you: |
synasapmob
force-pushed
the
feat/ENG-1339
branch
from
502fd63 to
4b391f9
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 19 changed files in this pull request and generated 7 comments.
Files not reviewed (1)
- frontend/pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (2)
frontend/app/utils/index.tsx:168
downloadFileWithBlobbuildstypewithout a leading dot and then strips that substring fromfileNameviareplace(type, ""). This breaks filenames that don’t already end with that extension (or contain the substring earlier), producing incorrect downloads (e.g.,report->reportpdf,png-banner.png->-banner.pngpng). Consider restoring the leading dot and/or stripping only a trailing extension (e.g., via a suffix check or regex) before settinganchor.download.
export const downloadFileWithBlob = (
blob: Blob,
mimeType: string,
fileName: string,
) => {
const blobUrl = URL.createObjectURL(blob);
const type = `${extension(mimeType)}`; // convert mime to correct type, image/png => png
const name = fileName.replace(type, ""); // no duplicate name, banner.png => banner
const anchor = document.createElement("a");
anchor.href = blobUrl;
anchor.download = `${name}${type}`;
frontend/app/layout/Artifact/ArtifactFile/ArtifactFileList/index.tsx:74
- The preview link builds the query string with
?file=${meta.name}without URL-encoding. Filenames containing spaces,&,#,?, or=will produce an invalid/ambiguous URL and break selection. UseencodeURIComponent(meta.name)(orcreateSearchParams) when constructing thetovalue.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (artifact.data.artifact.files.length === 1 && getSingleFile) { | ||
| return renderSectionFile(getSingleFile.mimeType, { | ||
| csv: <ArtifactFileCSV file={getSingleFile} />, | ||
| svg: <ArtifactFileSVG file={getSingleFile} />, | ||
| image: ( | ||
| <img | ||
| src={utilsWalrus.getQuiltPatchId(getSingleFile.patchId)} | ||
| alt={getSingleFile.name} | ||
| className="aspect-video object-cover" | ||
| /> | ||
| ); | ||
| } | ||
|
|
||
| if (getSingleFile?.mimeType?.startsWith?.("video")) { | ||
| return ( | ||
| ), | ||
| video: ( | ||
| <video | ||
| src={utilsWalrus.getQuiltPatchId(getSingleFile.patchId)} | ||
| controls={true} | ||
| className="aspect-video object-cover" | ||
| /> | ||
| ); | ||
| } | ||
|
|
||
| if (getSingleFile?.mimeType === "text/markdown") { | ||
| return <ArtifactFileMarkdown file={getSingleFile} />; | ||
| } | ||
|
|
||
| if (getSingleFile?.mimeType === "application/pdf") { | ||
| return <ArtifactFilePDF file={getSingleFile} />; | ||
| } | ||
| ), | ||
| markdown: getREADME ? ( | ||
| <ArtifactFileReadme file={getREADME} /> | ||
| ) : ( | ||
| <ArtifactFileMarkdown file={getSingleFile} /> | ||
| ), | ||
| pdf: <ArtifactFilePDF file={getSingleFile} />, | ||
| }); | ||
| } |
There was a problem hiding this comment.
When the artifact has exactly one file, the UI returns renderSectionFile(...) directly. For any unsupported/unknown mimeType (e.g., zip or plain text), renderSectionFile returns undefined, leaving the page blank instead of falling back to the file list/download UI. Add a fallback render when renderSectionFile returns nothing.
| {renderSectionFile(getSelectFile.mimeType, { | ||
| csv: <ArtifactFileCSV file={getSelectFile} />, | ||
| svg: <ArtifactFileSVG file={getSelectFile} />, | ||
| image: ( | ||
| <img | ||
| src={utilsWalrus.getQuiltPatchId(getSelectFile.patchId)} | ||
| alt={getSelectFile.name} | ||
| className="aspect-video object-cover" | ||
| /> | ||
| ), | ||
| video: ( | ||
| <video | ||
| src={utilsWalrus.getQuiltPatchId(getSelectFile.patchId)} | ||
| controls={true} | ||
| className="aspect-video object-cover" | ||
| /> | ||
| ), | ||
| markdown: <ArtifactFileMarkdown file={getSelectFile} />, | ||
| pdf: <ArtifactFilePDF file={getSelectFile} />, | ||
| })} |
There was a problem hiding this comment.
For selected files with an unsupported mimeType, renderSectionFile(...) returns undefined, so the preview area becomes empty (no message and no alternative action). Consider rendering an explicit “no preview available” state (and/or a download action) when renderSectionFile doesn’t match.
| <Typography>404 - page not found</Typography> | ||
|
|
||
| <Typography> | ||
| the list file does not contain the path | ||
| <Typography variant="span" className="text-white font-medium"> | ||
| {select} | ||
| </Typography> | ||
| </Typography> |
There was a problem hiding this comment.
The 404 copy has capitalization/grammar issues (“404 - page not found”, “the list file does not contain the path ...”). Consider using consistent capitalization and clearer phrasing (e.g., “404 — Page not found” and “The file list does not contain the path …”).
| ## Repository Structure (Expected) | ||
|
|
||
| - `components/` — reusable UI components | ||
| - `pages/` — route-level pages | ||
| - `hooks/` — custom hooks | ||
| - `services/` — API / GraphQL / blockchain logic | ||
| - `utils/` — helpers | ||
| - `assets/` — static files (SVGs, etc.) | ||
| - `docs/` — documentation | ||
|
|
There was a problem hiding this comment.
This instructions doc claims the repo uses components/, pages/, hooks/, etc., but the actual frontend is organized under frontend/app/... (e.g., app/components, app/layout, app/hook, app/services). Updating this section to match the real structure will prevent contributors from following the wrong paths.
| ## Repository Structure (Expected) | |
| - `components/` — reusable UI components | |
| - `pages/` — route-level pages | |
| - `hooks/` — custom hooks | |
| - `services/` — API / GraphQL / blockchain logic | |
| - `utils/` — helpers | |
| - `assets/` — static files (SVGs, etc.) | |
| - `docs/` — documentation | |
| ## Repository Structure | |
| Frontend code is organized under `frontend/app/`. | |
| - `frontend/app/components/` — reusable UI components | |
| - `frontend/app/layout/` — shared layout structure and wrappers | |
| - `frontend/app/hook/` — custom hooks | |
| - `frontend/app/services/` — API / GraphQL / blockchain logic |
| ## SVG Usage Rules | ||
|
|
||
| ### Rendering | ||
|
|
||
| All SVGs must use the **react-inlinesvg** library. | ||
|
|
||
| ```tsx | ||
| import SVG from "react-inlinesvg"; | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ### Do Not Use `<img>` for SVG | ||
|
|
||
| Never render SVG icons using: | ||
|
|
||
| ```tsx | ||
| <img src="/assets/icon.svg" /> | ||
| ``` | ||
|
|
||
| Using `react-inlinesvg` allows Tailwind classes to control: | ||
|
|
||
| - color | ||
| - size | ||
|
|
||
| --- | ||
|
|
||
| ### Do Not Inline Raw SVG | ||
|
|
||
| Avoid placing raw SVG markup directly inside JSX. | ||
|
|
||
| **Bad:** | ||
|
|
||
| ```tsx | ||
| <svg> | ||
| <path d="..." /> | ||
| </svg> | ||
| ``` | ||
|
|
||
| Instead: | ||
|
|
||
| - store SVG files in `/public/assets/` | ||
| - load them using `react-inlinesvg` | ||
|
|
There was a problem hiding this comment.
The SVG guidance says to use react-inlinesvg, but the codebase currently imports SVGs as React components via vite-plugin-svgr (e.g., import CloseLine from "public/assets/line/close.svg";). This doc should be corrected to reflect the actual convention (or the codebase should be changed to match, but that’s a larger refactor).
| ## Testing | ||
|
|
||
| - Use **Jest** | ||
| - Write tests for: | ||
| - utilities | ||
| - hooks | ||
| - critical logic | ||
|
|
There was a problem hiding this comment.
The Testing section states Jest is used, but frontend/package.json has no test script and there are no obvious Jest dependencies configured. If tests aren’t set up, this section should be updated (or add the missing test setup/scripts).
|
|
||
| return ( | ||
| <Drawer open={open} onOpenChange={setOpen} direction="left"> | ||
| <DrawerTrigger className="size-8 text-white/80 bg-white/14 rounded-sm min-[992px]:hidden"> |
There was a problem hiding this comment.
DrawerTrigger renders only an icon, with no accessible label/text. Add an aria-label (or visually hidden text) so screen readers can identify what this control does (e.g., “Open file list”).
| <DrawerTrigger className="size-8 text-white/80 bg-white/14 rounded-sm min-[992px]:hidden"> | |
| <DrawerTrigger | |
| aria-label="Open file list" | |
| className="size-8 text-white/80 bg-white/14 rounded-sm min-[992px]:hidden" | |
| > |
Screen.Recording.2026-04-06.at.17.17.46.mov