feat(platform): add copilot artifact preview panel - #12629
Conversation
## Summary Upgrade the frontend **Docker image** from **Node.js v21** (EOL since June 2024) to **Node.js v22 LTS** (supported through April 2027). > **Scope:** This only affects the **Dockerfile** used for local development (`docker compose`) and CI. It does **not** affect Vercel (which manages its own Node.js runtime) or Kubernetes (the frontend Helm chart was removed in Dec 2025 — the frontend is deployed exclusively via Vercel). ## Why - Node v21.7.3 has a **known TransformStream race condition bug** causing `TypeError: controller[kState].transformAlgorithm is not a function` — this is [BUILDER-3KF](https://significant-gravitas.sentry.io/issues/BUILDER-3KF) with **567,000+ Sentry events** - The error is entirely in Node.js internals (`node:internal/webstreams/transformstream`), zero first-party code - Node 21 is **not an LTS release** and has been EOL since June 2024 - `package.json` already declares `"engines": { "node": "22.x" }` — the Dockerfile was inconsistent - Node 22.x LTS (v22.22.1) fixes the TransformStream bug - Next.js 15.4.x requires Node 18.18+, so Node 22 is fully compatible ## Changes - `autogpt_platform/frontend/Dockerfile`: `node:21-alpine` → `node:22.22-alpine3.23` (both `base` and `prod` stages) ## Test plan - [ ] Verify frontend Docker image builds successfully via `docker compose` - [ ] Verify frontend starts and serves pages correctly in local Docker environment - [ ] Monitor Sentry for BUILDER-3KF — should drop to zero for Docker-based runs
github-project-automation
Bot
moved this to 🆕 Needs initial review
in AutoGPT development kanban
|
This PR targets the Automatically setting the base branch to |
github-actions
Bot
added
platform/frontend
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds workspace file metadata persistence and a new GET /api/workspace/files endpoint; introduces a resizable Artifact Panel with artifact classification, preview/renderers, store support, auto-open logic, message→artifact extraction, and related UI components and tests. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Frontend as Frontend (React)
participant Store as Copilot Store
participant Backend as Backend API
participant DB as WorkspaceManager/DB
User->>Frontend: Upload file / agent creates file
Frontend->>Backend: POST/write request (includes metadata.origin)
Backend->>DB: WorkspaceManager.write_file(content, metadata)
DB-->>Backend: persisted file record
Backend->>Frontend: assistant message containing workspace://<fileID>
Frontend->>Frontend: extractWorkspaceArtifacts(message)
Frontend->>Store: openArtifact(ArtifactRef)
Store->>Store: set artifactPanel.isOpen, activeArtifact, history
Frontend->>Backend: GET /api/workspace/files or download proxy
Backend->>DB: fetch file blob/metadata
DB-->>Backend: file content/metadata
Backend-->>Frontend: file content
Frontend->>Frontend: classifyArtifact(...) → ArtifactContent renders (image/pdf/html/react/code/csv/text)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
github-actions
Bot
added
the
conflicts
|
This pull request has conflicts with the base branch, please resolve those so we can evaluate the pull request. |
Merge origin/dev into feature branch, resolving conflict in CopilotPage.tsx — kept artifact panel layout restructure while adding isSyncing and historicalDurations props from dev. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
github-actions
Bot
removed
the
conflicts
|
Conflicts have been resolved! 🎉 A maintainer will review the pull request shortly. |
🔍 PR Overlap DetectionThis check compares your PR against all other open PRs targeting the same branch to detect potential merge conflicts early. 🔴 Merge Conflicts DetectedThe following PRs have been tested and will have merge conflicts if merged after this PR. Consider coordinating with the authors.
🟢 Low Risk — File Overlap OnlyThese PRs touch the same files but different sections (click to expand)
Summary: 6 conflict(s), 0 medium risk, 1 low risk (out of 7 PRs with file overlap) Auto-generated on push. Ignores: |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
autogpt_platform/backend/backend/copilot/tools/workspace_files.py (1)
808-818:⚠️ Potential issue | 🟡 MinorDuplicate virus scan detected.
scan_content_safeis called explicitly at line 809, butWorkspaceManager.write_file(line 188-190 in workspace.py) also callsscan_content_safeinternally with the comment "Callers must NOT duplicate this scan."Remove the redundant scan here to avoid scanning the same content twice.
🛠️ Proposed fix
try: - await scan_content_safe(content, filename=filename) manager = await get_workspace_manager(user_id, session_id) rec = await manager.write_file(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@autogpt_platform/backend/backend/copilot/tools/workspace_files.py` around lines 808 - 818, Remove the redundant virus scan by deleting the explicit call to scan_content_safe before obtaining the manager; the WorkspaceManager.write_file implementation already performs scan_content_safe (see WorkspaceManager.write_file and its comment "Callers must NOT duplicate this scan"), so call get_workspace_manager(user_id, session_id) and then manager.write_file(...) directly (preserve passing content, filename, path, mime_type, overwrite, metadata) to avoid double-scanning.
🧹 Nitpick comments (7)
autogpt_platform/backend/backend/api/features/workspace/routes.py (1)
311-345: Consider splitting this route module; it is now beyond the target size.Adding this endpoint pushes the file to ~345 lines. Extracting file-list response mapping/helpers (or moving list/download/upload handlers into focused modules) would keep this easier to maintain.
As per coding guidelines: “Keep files under ~300 lines; if a file grows beyond this, split by responsibility.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@autogpt_platform/backend/backend/api/features/workspace/routes.py` around lines 311 - 345, The file has grown past the target size because route handlers and inline response mapping are mixed; extract responsibilities by moving the list workspace file response mapping and any related helpers into a new module and/or moving related upload/download/list route handlers into a dedicated workspace routes package. Concretely: create a new module (e.g., workspace/serializers or workspace/utils) to host the ListFilesResponse mapping logic currently inside list_workspace_files and any helper functions that transform WorkspaceManager file objects to dicts, update list_workspace_files to import and call that mapper, and consider moving other handlers that use WorkspaceManager (upload/download) into a focused routes file so autogpt_platform.backend.api.features.workspace.routes only wires endpoints. Ensure you keep the function name list_workspace_files, the ListFilesResponse shape, and the WorkspaceManager usage intact while replacing inline mapping with an import from the new module.autogpt_platform/frontend/src/app/(platform)/copilot/components/ChatMessagesContainer/components/MessagePartRenderer.tsx (1)
74-92:useMemousage without explicit optimization request.Per coding guidelines,
useMemoshould not be used unless specifically asked to optimize. TheextractWorkspaceArtifactscall is memoized whileresolveWorkspaceUrlson line 76 is not, creating an inconsistent approach. If memoization is truly needed for performance, both should be memoized; otherwise, remove theuseMemo.♻️ Remove useMemo for consistency
function TextWithArtifactCards({ text }: { text: string }) { - const artifacts = useMemo(() => extractWorkspaceArtifacts(text), [text]); + const artifacts = extractWorkspaceArtifacts(text); const resolved = resolveWorkspaceUrls(text);As per coding guidelines: "Do not use
useCallbackoruseMemounless asked to optimize a given function"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@autogpt_platform/frontend/src/app/`(platform)/copilot/components/ChatMessagesContainer/components/MessagePartRenderer.tsx around lines 74 - 92, In TextWithArtifactCards, remove the unnecessary useMemo around extractWorkspaceArtifacts to follow the guideline against using useMemo/useCallback unless optimizing; call extractWorkspaceArtifacts(text) directly (so artifacts is computed like const artifacts = extractWorkspaceArtifacts(text)) and keep resolveWorkspaceUrls(text) as-is (or alternatively memoize both if you truly need optimization), referencing the TextWithArtifactCards function and the extractWorkspaceArtifacts and resolveWorkspaceUrls calls.autogpt_platform/frontend/src/app/(platform)/copilot/components/ArtifactPanel/components/ArtifactDragHandle.tsx (1)
21-53:useCallbackused without explicit optimization request.Per coding guidelines,
useCallbackshould not be used unless specifically asked to optimize. Since the handler attaches/removes document listeners within itself, a regular function declaration would work correctly without stale closure issues.♻️ Convert to function declaration
- const handlePointerDown = useCallback( - (e: React.PointerEvent) => { + function handlePointerDown(e: React.PointerEvent) { e.preventDefault(); setIsDragging(true); startXRef.current = e.clientX; // Get the panel's current width from its parent const panel = (e.target as HTMLElement).closest( "[data-artifact-panel]", ) as HTMLElement | null; startWidthRef.current = panel?.offsetWidth ?? 600; const handlePointerMove = (moveEvent: PointerEvent) => { const delta = startXRef.current - moveEvent.clientX; const maxWidth = window.innerWidth * (maxWidthPercent / 100); const newWidth = Math.min( maxWidth, Math.max(minWidth, startWidthRef.current + delta), ); onWidthChange(newWidth); }; const handlePointerUp = () => { setIsDragging(false); document.removeEventListener("pointermove", handlePointerMove); document.removeEventListener("pointerup", handlePointerUp); }; document.addEventListener("pointermove", handlePointerMove); document.addEventListener("pointerup", handlePointerUp); - }, - [onWidthChange, minWidth, maxWidthPercent], - ); + }As per coding guidelines: "Do not use
useCallbackoruseMemounless asked to optimize a given function"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@autogpt_platform/frontend/src/app/`(platform)/copilot/components/ArtifactPanel/components/ArtifactDragHandle.tsx around lines 21 - 53, Replace the useCallback-wrapped handlePointerDown with a plain function declaration named handlePointerDown (remove the useCallback import/use) and keep the internal logic that sets startXRef/startWidthRef and attaches handlePointerMove and handlePointerUp listeners; ensure handlePointerMove and handlePointerUp remain local functions so they close over the current minWidth, maxWidthPercent, and onWidthChange values and that document.removeEventListener is called in handlePointerUp to clean up listeners, leaving refs (startXRef, startWidthRef) and setIsDragging usage unchanged.autogpt_platform/frontend/src/app/(platform)/copilot/CopilotPage.tsx (1)
16-23: Dynamic import placed in the middle of static imports.The dynamic import for
ArtifactPanelis placed between two static import blocks, which can make the import section harder to scan. Consider grouping it with other imports or moving it after all static imports for consistency.♻️ Suggested reorganization
import dynamic from "next/dynamic"; import { useCallback, useEffect, useRef, useState } from "react"; import { ChatContainer } from "./components/ChatContainer/ChatContainer"; import { ChatSidebar } from "./components/ChatSidebar/ChatSidebar"; - -const ArtifactPanel = dynamic( - () => - import("./components/ArtifactPanel/ArtifactPanel").then( - (m) => m.ArtifactPanel, - ), - { ssr: false }, -); import { DeleteChatDialog } from "./components/DeleteChatDialog/DeleteChatDialog"; import { MobileDrawer } from "./components/MobileDrawer/MobileDrawer"; import { MobileHeader } from "./components/MobileHeader/MobileHeader"; import { NotificationBanner } from "./components/NotificationBanner/NotificationBanner"; import { NotificationDialog } from "./components/NotificationDialog/NotificationDialog"; import { RateLimitResetDialog } from "./components/RateLimitResetDialog/RateLimitResetDialog"; import { ScaleLoader } from "./components/ScaleLoader/ScaleLoader"; import { useCopilotPage } from "./useCopilotPage"; + +const ArtifactPanel = dynamic( + () => + import("./components/ArtifactPanel/ArtifactPanel").then( + (m) => m.ArtifactPanel, + ), + { ssr: false }, +);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@autogpt_platform/frontend/src/app/`(platform)/copilot/CopilotPage.tsx around lines 16 - 23, Move the dynamic import for ArtifactPanel so it is not interleaved with static imports: place the dynamic call to dynamic(() => import("./components/ArtifactPanel/ArtifactPanel").then(m => m.ArtifactPanel), { ssr: false }) after all other static imports (e.g., after the DeleteChatDialog import) or group it with other dynamic imports if present; update CopilotPage.tsx to keep all static imports together at the top and ensure only dynamic() calls (like ArtifactPanel) appear in a separate block to improve scanability.autogpt_platform/frontend/src/app/(platform)/copilot/components/ArtifactPanel/components/ArtifactMinimizedStrip.tsx (1)
33-44: Inline styles for vertical text are acceptable here.The
writingModeandtextOrientationCSS properties don't have standard Tailwind equivalents, so inline styles are a reasonable approach. However,maxHeight,overflow, andtextOverflowcould be converted to Tailwind classes if you want to minimize inline styles.♻️ Partial Tailwind migration (optional)
<span - className="mt-2 text-xs text-zinc-400" + className="mt-2 max-h-[120px] overflow-hidden text-ellipsis text-xs text-zinc-400" style={{ writingMode: "vertical-rl", textOrientation: "mixed", - maxHeight: "120px", - overflow: "hidden", - textOverflow: "ellipsis", }} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@autogpt_platform/frontend/src/app/`(platform)/copilot/components/ArtifactPanel/components/ArtifactMinimizedStrip.tsx around lines 33 - 44, Replace the inline style properties maxHeight, overflow, and textOverflow in the span inside ArtifactMinimizedStrip (the span that renders {artifact.title}) with Tailwind classes: use max-h-[120px] for maxHeight, overflow-hidden for overflow, and truncate (plus ensure the element is a block/inline-block if needed) for textOverflow: "ellipsis"; keep writingMode and textOrientation inline since Tailwind has no standard equivalents.autogpt_platform/frontend/src/app/(platform)/copilot/components/ArtifactPanel/components/ArtifactContent.tsx (1)
127-134: Use the shared error surface here.This custom fallback bypasses the standard Copilot error presentation. Swapping it to
<ErrorCard />will keep preview failures consistent with the rest of the frontend.As per coding guidelines, "Use '' component for rendering errors in frontend UI; use toast notifications for mutation errors; use 'Sentry.captureException()' for manual exceptions".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@autogpt_platform/frontend/src/app/`(platform)/copilot/components/ArtifactPanel/components/ArtifactContent.tsx around lines 127 - 134, Replace the custom JSX error fallback in ArtifactContent (the if (error) return { ... } block) with the shared ErrorCard component so preview failures use the standard Copilot error surface; locate the error branch inside ArtifactContent.tsx and render <ErrorCard /> (passing any required props like message or details from the error variable) instead of the current div, and remove the custom text nodes to ensure consistent error presentation across the frontend.autogpt_platform/frontend/src/app/(platform)/copilot/components/ArtifactPanel/components/reactArtifactPreview.ts (1)
103-104: Use production React builds and pin to exact stable versions.The preview currently loads React development builds from unpkg. For better performance and smaller bundle size, use production builds instead. Also pin to exact versions (e.g.,
react@18.3.1) to prevent unexpected breakage from minor updates.Suggested change
- <script crossorigin src="https://unpkg.com/react@18/umd/react.development.js"></script> - <script crossorigin src="https://unpkg.com/react-dom@18/umd/react-dom.development.js"></script> + <script crossorigin src="https://unpkg.com/react@18.3.1/umd/react.production.min.js"></script> + <script crossorigin src="https://unpkg.com/react-dom@18.3.1/umd/react-dom.production.min.js"></script>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@autogpt_platform/frontend/src/app/`(platform)/copilot/components/ArtifactPanel/components/reactArtifactPreview.ts around lines 103 - 104, The script tags in reactArtifactPreview.ts currently load unpinned React development builds; update the two <script> tags that reference react and react-dom in the reactArtifactPreview component to point to the production UMD builds and pin to exact stable versions (for example use react@18.3.1 and react-dom@18.3.1 production UMD URLs) while preserving crossorigin attribute and integrity if you add SRI—i.e., replace the development URLs with the corresponding production URLs and exact version numbers to reduce size and avoid unexpected upgrades.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@autogpt_platform/backend/backend/api/features/workspace/routes.py`:
- Around line 134-136: Replace the untyped files: list[dict] in
ListFilesResponse with a typed Pydantic model (e.g. create a FileItem(BaseModel)
with the exact fields used in the dict mapping at the response assembly block)
and change files to use list[FileItem]; also update the code that builds the
dicts (the mapping at the block referencing lines 331-343) to instantiate
FileItem(...) objects (or use FileItem.parse_obj) so the response returns typed
models instead of raw dicts and the OpenAPI/client types are generated
correctly.
In
`@autogpt_platform/frontend/src/app/`(platform)/copilot/components/ArtifactPanel/ArtifactPanel.tsx:
- Around line 43-57: The header minimize/maximize controls update store state
(isMinimized, effectiveWidth via minimizeArtifactPanel/maximizeArtifactPanel)
but the mobile UI always renders SheetContent and ignores those states; either
hide/disable those header actions on mobile by checking the mobile rendering
branch (e.g., use isMobile or where SheetContent is chosen) so headerProps omits
onMinimize/onMaximize/onRestore and related buttons, or make the Sheet component
(the mobile branch that renders SheetContent) read isMinimized and
effectiveWidth and apply the corresponding collapsed/maximized styles/behavior;
update the rendering logic where headerProps is assembled and where SheetContent
is rendered so the mobile sheet either respects isMinimized/effectiveWidth or
the header actions are not passed/are no-ops.
In
`@autogpt_platform/frontend/src/app/`(platform)/copilot/components/ArtifactPanel/components/ArtifactContent.tsx:
- Around line 60-89: The PDF fetch creates an objectUrl after the component has
been cancelled but cleanup already ran, leaking the blob URL; inside the
.then(blob => { ... }) handler in ArtifactContent (where objectUrl and cancelled
are used) revoke the created URL if cancelled is true instead of only skipping
state updates: i.e., after objectUrl = URL.createObjectURL(blob) check cancelled
— if false setPdfUrl(objectUrl) and setIsLoading(false), but if true immediately
call URL.revokeObjectURL(objectUrl) to clean up; the same pattern applies in the
.catch handler where a URL might have been created before error handling.
- Around line 30-45: The restore-effect should wait until the new content has
mounted (i.e., after loading finishes) and treat saved 0 as valid; update the
second useEffect that reads scrollPositions.current.get(artifact.id) so it
depends on both artifact.id and the loading flag (e.g., isLoading / loading /
isFetching) and only runs when loading is false; inside the effect read saved
with a strict undefined check (const saved =
scrollPositions.current.get(artifact.id); const top = saved !== undefined ?
saved : 0) and then set scrollRef.current.scrollTop = top when scrollRef.current
exists. Ensure you reference artifact.id, scrollRef, and scrollPositions in the
change.
In
`@autogpt_platform/frontend/src/app/`(platform)/copilot/components/ArtifactPanel/components/ArtifactPanelHeader.tsx:
- Around line 33-51: HeaderButton uses the title prop as a tooltip but lacks an
explicit accessible name; update the HeaderButton component (function
HeaderButton) to add aria-label={title} on the rendered <button> element so the
icon-only controls (back/copy/download/minimize/maximize/close) are announced
consistently by assistive tech while keeping title for hover tooltips.
In
`@autogpt_platform/frontend/src/app/`(platform)/copilot/components/ArtifactPanel/useArtifactPanel.ts:
- Around line 53-58: handleCopy currently assumes every artifact is text by
always calling res.text(); change it to only perform a text copy for text-based
artifacts (inspect activeArtifact.mimeType or
response.headers.get('content-type') and allow text/*, application/json, etc.),
otherwise route the action to the renderer's copy contract (e.g., call the
platform copy handler like copyArtifact or emit an event) so PDFs/images
implement their own behavior; also await the fetch and
navigator.clipboard.writeText calls and add proper error handling/logging (catch
and surface failures via the renderer or UI) so failures aren't dropped
silently.
In
`@autogpt_platform/frontend/src/app/`(platform)/copilot/components/ChatContainer/useAutoOpenArtifacts.ts:
- Around line 22-25: The current reset in the useEffect for sessionId resets
messageFingerprintsRef and hasInitializedRef which causes the first live
assistant artifact to be treated as hydration and skipped; introduce a separate
ref (e.g., hasHydratedRef) to represent "history finished hydrating" distinct
from hasInitializedRef, set hasHydratedRef to true once the initial messages
array has been processed/hydrated (watch the messages prop or the hydration
completion path), and update the artifact-handling logic to use hasHydratedRef
to decide whether an incoming assistant artifact is live vs. hydration while
leaving hasInitializedRef for dedup/fingerprint logic (references: useEffect,
messageFingerprintsRef.current, hasInitializedRef, sessionId, and any artifact
handler that checks hasInitializedRef).
In
`@autogpt_platform/frontend/src/components/contextual/OutputRenderers/renderers/CSVRenderer.tsx`:
- Around line 126-131: canRenderCSV currently misses filenames with uppercase
extensions because it checks metadata?.filename?.endsWith(".csv")
case-sensitively; update the function (canRenderCSV) to normalize the filename
to lowercase before checking the extension (e.g., use
metadata.filename.toLowerCase().endsWith(".csv") with a safe guard for
undefined) so it mirrors the case-insensitive behavior used in canRenderHTML and
correctly recognizes files like DATA.CSV.
---
Outside diff comments:
In `@autogpt_platform/backend/backend/copilot/tools/workspace_files.py`:
- Around line 808-818: Remove the redundant virus scan by deleting the explicit
call to scan_content_safe before obtaining the manager; the
WorkspaceManager.write_file implementation already performs scan_content_safe
(see WorkspaceManager.write_file and its comment "Callers must NOT duplicate
this scan"), so call get_workspace_manager(user_id, session_id) and then
manager.write_file(...) directly (preserve passing content, filename, path,
mime_type, overwrite, metadata) to avoid double-scanning.
---
Nitpick comments:
In `@autogpt_platform/backend/backend/api/features/workspace/routes.py`:
- Around line 311-345: The file has grown past the target size because route
handlers and inline response mapping are mixed; extract responsibilities by
moving the list workspace file response mapping and any related helpers into a
new module and/or moving related upload/download/list route handlers into a
dedicated workspace routes package. Concretely: create a new module (e.g.,
workspace/serializers or workspace/utils) to host the ListFilesResponse mapping
logic currently inside list_workspace_files and any helper functions that
transform WorkspaceManager file objects to dicts, update list_workspace_files to
import and call that mapper, and consider moving other handlers that use
WorkspaceManager (upload/download) into a focused routes file so
autogpt_platform.backend.api.features.workspace.routes only wires endpoints.
Ensure you keep the function name list_workspace_files, the ListFilesResponse
shape, and the WorkspaceManager usage intact while replacing inline mapping with
an import from the new module.
In
`@autogpt_platform/frontend/src/app/`(platform)/copilot/components/ArtifactPanel/components/ArtifactContent.tsx:
- Around line 127-134: Replace the custom JSX error fallback in ArtifactContent
(the if (error) return { ... } block) with the shared ErrorCard component so
preview failures use the standard Copilot error surface; locate the error branch
inside ArtifactContent.tsx and render <ErrorCard /> (passing any required props
like message or details from the error variable) instead of the current div, and
remove the custom text nodes to ensure consistent error presentation across the
frontend.
In
`@autogpt_platform/frontend/src/app/`(platform)/copilot/components/ArtifactPanel/components/ArtifactDragHandle.tsx:
- Around line 21-53: Replace the useCallback-wrapped handlePointerDown with a
plain function declaration named handlePointerDown (remove the useCallback
import/use) and keep the internal logic that sets startXRef/startWidthRef and
attaches handlePointerMove and handlePointerUp listeners; ensure
handlePointerMove and handlePointerUp remain local functions so they close over
the current minWidth, maxWidthPercent, and onWidthChange values and that
document.removeEventListener is called in handlePointerUp to clean up listeners,
leaving refs (startXRef, startWidthRef) and setIsDragging usage unchanged.
In
`@autogpt_platform/frontend/src/app/`(platform)/copilot/components/ArtifactPanel/components/ArtifactMinimizedStrip.tsx:
- Around line 33-44: Replace the inline style properties maxHeight, overflow,
and textOverflow in the span inside ArtifactMinimizedStrip (the span that
renders {artifact.title}) with Tailwind classes: use max-h-[120px] for
maxHeight, overflow-hidden for overflow, and truncate (plus ensure the element
is a block/inline-block if needed) for textOverflow: "ellipsis"; keep
writingMode and textOrientation inline since Tailwind has no standard
equivalents.
In
`@autogpt_platform/frontend/src/app/`(platform)/copilot/components/ArtifactPanel/components/reactArtifactPreview.ts:
- Around line 103-104: The script tags in reactArtifactPreview.ts currently load
unpinned React development builds; update the two <script> tags that reference
react and react-dom in the reactArtifactPreview component to point to the
production UMD builds and pin to exact stable versions (for example use
react@18.3.1 and react-dom@18.3.1 production UMD URLs) while preserving
crossorigin attribute and integrity if you add SRI—i.e., replace the development
URLs with the corresponding production URLs and exact version numbers to reduce
size and avoid unexpected upgrades.
In
`@autogpt_platform/frontend/src/app/`(platform)/copilot/components/ChatMessagesContainer/components/MessagePartRenderer.tsx:
- Around line 74-92: In TextWithArtifactCards, remove the unnecessary useMemo
around extractWorkspaceArtifacts to follow the guideline against using
useMemo/useCallback unless optimizing; call extractWorkspaceArtifacts(text)
directly (so artifacts is computed like const artifacts =
extractWorkspaceArtifacts(text)) and keep resolveWorkspaceUrls(text) as-is (or
alternatively memoize both if you truly need optimization), referencing the
TextWithArtifactCards function and the extractWorkspaceArtifacts and
resolveWorkspaceUrls calls.
In `@autogpt_platform/frontend/src/app/`(platform)/copilot/CopilotPage.tsx:
- Around line 16-23: Move the dynamic import for ArtifactPanel so it is not
interleaved with static imports: place the dynamic call to dynamic(() =>
import("./components/ArtifactPanel/ArtifactPanel").then(m => m.ArtifactPanel), {
ssr: false }) after all other static imports (e.g., after the DeleteChatDialog
import) or group it with other dynamic imports if present; update
CopilotPage.tsx to keep all static imports together at the top and ensure only
dynamic() calls (like ArtifactPanel) appear in a separate block to improve
scanability.
🪄 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: 380f1dc8-e7e3-4c0f-889e-764dfd33e5a1
📒 Files selected for processing (32)
autogpt_platform/backend/backend/api/features/workspace/routes.pyautogpt_platform/backend/backend/copilot/tools/workspace_files.pyautogpt_platform/backend/backend/util/workspace.pyautogpt_platform/frontend/src/app/(platform)/copilot/CopilotPage.tsxautogpt_platform/frontend/src/app/(platform)/copilot/components/ArtifactCard/ArtifactCard.tsxautogpt_platform/frontend/src/app/(platform)/copilot/components/ArtifactPanel/ArtifactPanel.tsxautogpt_platform/frontend/src/app/(platform)/copilot/components/ArtifactPanel/components/ArtifactContent.tsxautogpt_platform/frontend/src/app/(platform)/copilot/components/ArtifactPanel/components/ArtifactDragHandle.tsxautogpt_platform/frontend/src/app/(platform)/copilot/components/ArtifactPanel/components/ArtifactMinimizedStrip.tsxautogpt_platform/frontend/src/app/(platform)/copilot/components/ArtifactPanel/components/ArtifactPanelHeader.tsxautogpt_platform/frontend/src/app/(platform)/copilot/components/ArtifactPanel/components/ArtifactReactPreview.tsxautogpt_platform/frontend/src/app/(platform)/copilot/components/ArtifactPanel/components/SourceToggle.tsxautogpt_platform/frontend/src/app/(platform)/copilot/components/ArtifactPanel/components/reactArtifactPreview.test.tsautogpt_platform/frontend/src/app/(platform)/copilot/components/ArtifactPanel/components/reactArtifactPreview.tsautogpt_platform/frontend/src/app/(platform)/copilot/components/ArtifactPanel/helpers.test.tsautogpt_platform/frontend/src/app/(platform)/copilot/components/ArtifactPanel/helpers.tsautogpt_platform/frontend/src/app/(platform)/copilot/components/ArtifactPanel/useArtifactPanel.tsautogpt_platform/frontend/src/app/(platform)/copilot/components/ChatContainer/ChatContainer.tsxautogpt_platform/frontend/src/app/(platform)/copilot/components/ChatContainer/useAutoOpenArtifacts.test.tsautogpt_platform/frontend/src/app/(platform)/copilot/components/ChatContainer/useAutoOpenArtifacts.tsautogpt_platform/frontend/src/app/(platform)/copilot/components/ChatMessagesContainer/components/MessageAttachments.tsxautogpt_platform/frontend/src/app/(platform)/copilot/components/ChatMessagesContainer/components/MessagePartRenderer.tsxautogpt_platform/frontend/src/app/(platform)/copilot/components/ChatMessagesContainer/helpers.tsautogpt_platform/frontend/src/app/(platform)/copilot/store.test.tsautogpt_platform/frontend/src/app/(platform)/copilot/store.tsautogpt_platform/frontend/src/app/api/openapi.jsonautogpt_platform/frontend/src/components/contextual/OutputRenderers/index.tsautogpt_platform/frontend/src/components/contextual/OutputRenderers/renderers/CSVRenderer.tsxautogpt_platform/frontend/src/components/contextual/OutputRenderers/renderers/CodeRenderer.test.tsautogpt_platform/frontend/src/components/contextual/OutputRenderers/renderers/CodeRenderer.tsxautogpt_platform/frontend/src/components/contextual/OutputRenderers/renderers/HTMLRenderer.tsxautogpt_platform/frontend/src/services/storage/local-storage.ts
ntindle
changed the title
- Use typed WorkspaceFileItem Pydantic model instead of list[dict] - Gate copy action to text-based artifacts (skip images/PDFs/binaries) - Hide minimize/maximize buttons on mobile Sheet overlay - Add aria-label to all icon-only header buttons for accessibility - Fix case-sensitive .csv extension check in CSVRenderer Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
github-project-automation
Bot
moved this from 👍🏼 Mergeable
to ✅ Done
in AutoGPT development kanban
![autogpt-pr-reviewer-in-dev[bot]](https://avatars.githubusercontent.com/u/130738209?s=60&v=4){kind=small}
There was a problem hiding this comment.
❓ INCONCLUSIVE
You've hit your limit · resets 12pm (UTC)
Risk level: medium | Duration: 83s | Reviewed: b732d10e
Specialist Reports
| Specialist | Status | Summary |
|---|---|---|
| security | You've hit your limit · resets 12pm (UTC) | |
| architect | You've hit your limit · resets 12pm (UTC) | |
| performance | You've hit your limit · resets 12pm (UTC) | |
| testing | You've hit your limit · resets 12pm (UTC) | |
| quality | You've hit your limit · resets 12pm (UTC) | |
| product | You've hit your limit · resets 12pm (UTC) | |
| discussion | You've hit your limit · resets 12pm (UTC) | |
| ui-reviewer | You've hit your limit · resets 12pm (UTC) |
Quality Checks
- ❌ lint: cd autogpt_platform/frontend && pnpm lint:
cd autogpt_platform/frontend && pnpm lint(1s) - ✅ lint: cd autogpt_platform/backend && poetry run lint:
cd autogpt_platform/backend && poetry run lint(70s) - ❌ typecheck: cd autogpt_platform/frontend && pnpm types:
cd autogpt_platform/frontend && pnpm types(0s) - ❌ test: cd autogpt_platform/backend && poetry run test:
cd autogpt_platform/backend && poetry run test(0s) - ❌ test: cd autogpt_platform/frontend && pnpm test:unit:
cd autogpt_platform/frontend && pnpm test:unit(0s) - ❌ build: cd autogpt_platform/frontend && pnpm build:
cd autogpt_platform/frontend && pnpm build(0s)
There was a problem hiding this comment.
📋 Automated Review — PR #12629
PR #12629 — feat(platform): add copilot artifact preview panel
Author: ntindle | Files: 45
🎯 Verdict: REQUEST_CHANGES
PR Description Quality
✅ Has Why + What + How — PR description clearly explains the feature (artifact preview panel for copilot), lists all major components added, and includes screenshots/video. Checklist is complete.
What This PR Does
Adds a dedicated artifact preview panel to the copilot interface that can render AI-generated content — HTML pages, React components, PDFs, CSVs, images, and code — inside sandboxed iframes. The panel supports drag-to-resize, minimize/maximize, history navigation, auto-open on new artifacts, and mobile Sheet overlay. Backend adds a paginated GET /workspace/files endpoint and tracks file origin metadata (user-upload vs agent-created).
Specialist Findings
🛡️ Security sandbox="allow-scripts" without allow-same-origin, React CDN bundles have SRI hashes, and the feature is gated behind Flag.ARTIFACTS. However, two high-severity gaps remain.
- 🟠 PDF iframe unsandboxed (
ArtifactContent.tsx:120) — Nosandboxattribute at all. While a blob: URL gives a null origin, an unsandboxed iframe can trigger top-level navigations, open popups, and submit forms. Discussion shows @majdyz flagged this and it was reportedly fixed in commit 48354ac withsandbox="", but the security specialist's final analysis still flags the implementation. Needs verification that the fix landed correctly. - 🟠 AI-generated code can probe internal networks (
reactArtifactPreview.ts:14-16) — fetch/XHR/WebSocket are explicitly allowed from sandboxed iframes. AI-generated artifacts could scan localhost, internal IP ranges, or cloud metadata endpoints (169.254.169.254) from the user's browser. - 🟡 Tailwind CDN without SRI (
iframe-sandbox-csp.ts:32) — Tailwind JIT CDN loaded without integrity hash in every preview iframe. A CDN compromise would execute attacker JS in all artifact previews. (Flagged by: security — 1) - 🟡 Internal path leakage (
routes.py:375) —WorkspaceFileItemexposes session-scoped paths like/sessions/<session_id>/filename.txtto the frontend. (Flagged by: security — 1)
🏗️ Architecture ✅ — Well-decomposed: classification (helpers.ts), content fetching (useArtifactContent), transpilation (transpileReactArtifact), rendering (ArtifactContent), and downloads (downloadArtifact) are cleanly separated. Follows established component/hook/helpers pattern from CONTRIBUTING.md. Feature-flagged correctly.
- 🟠 Error handling via string matching (
routes.py:279-282) — HTTP status 413 vs 409 determined bymessage.startswith("File too large"). If the error message text changes, the wrong status code is returned. Should use a custom exception hierarchy. (Flagged by: architect — 1) - 🔵 Barrel file import (
ArtifactContent.tsx:3) — Imports fromOutputRenderers/index.tswhich AGENTS.md says to avoid.
⚡ Performance ✅ — Good lazy-loading (TypeScript compiler), bounded LRU cache, tail-scan optimization for auto-open, debounced localStorage persistence. No blocking issues.
- 🟡 Drag resize re-renders (
ArtifactDragHandle.tsx:43) — Everypointermovetriggers a Zustand update + React re-render of the full panel tree including iframes. ConsiderrequestAnimationFramethrottling or CSS variable mutation during drag. (Flagged by: performance — 1) - 🟡 PDF blobs not cached (
useArtifactContent.ts:79) — PDFs re-fetched on every artifact switch unlike text artifacts. (Flagged by: performance — 1)
🧪 Testing
- 🟠
useArtifactPanelhas zero tests (useArtifactPanel.ts:89) — Contains copy-to-clipboard logic (cached vs fetch fallback),canCopygating, Escape key handler, and width clamping — all testable viarenderHook. (Flagged by: testing — 1) - 🟠
ArtifactContentrenderer routing untested (ArtifactContent.tsx:76) — ~8 branches (image, pdf, html, react, code, json, csv, fallback) with no direct test. A routing regression (e.g., code rendered as markdown) would go undetected. (Flagged by: testing — 1) - 🟠 CSVRenderer assertions are smoke-only (
CSVRenderer.test.ts:48-66) — All five render tests only assertnot.toThrow(). A broken parser returning empty rows would pass. PR claims RFC 4180 compliance but no assertion verifies parsed cell content. (Flagged by: testing — 1) - 🟠 Backend agent metadata untested (
workspace_files.py:848) —metadata={"origin": "agent-created"}has no test to verify it's passed correctly. (Flagged by: testing — 1)
📖 Quality ✅ — Clean naming, good TypeScript usage (no any), excellent security documentation in iframe-sandbox-csp.ts and reactArtifactPreview.ts. One convention violation.
- 🟠 Arrow function component (
ChatContainer.tsx:37) —ChatContainerusesexport const ChatContainer = (…) => {violating the codebase convention "Use function declarations for components/handlers." (Flagged by: quality — 1) - 🔵 Duplicated width constants (
store.ts:64-65,ArtifactDragHandle.tsx:17-18) — min-width (320) and max-width-percent (85) defined independently in two files.
📦 Product ✅ — Core flows are solid with error + retry UI, loading skeletons, mobile Sheet overlay, and proper feature gating. Minor polish items.
- 🟡 Image renderer lacks error/loading state (
ArtifactContent.tsx:84) — Bare<img>with noonErrorhandler or skeleton, unlike text/PDF artifacts which have retry UI. (Flagged by: product — 1) - 🟡 Drag handle keyboard inaccessible (
ArtifactDragHandle.tsx:73) —role="separator"without arrow-key handler oraria-valuenow. Screen reader users can focus but cannot resize. (Flagged by: product — 1) - 🟡 Generic 404 error for deleted artifacts (
ArtifactContent.tsx:40) — Shows "Failed to fetch: 404" with a retry button that will never succeed for deleted files. (Flagged by: product — 1)
📬 Discussion
- 🟠 CSVTable renders unbounded rows (
CSVRenderer.tsx:112) — @majdyz flagged that a 100k-row CSV creates 100k+ DOM nodes. No cap or virtualization added. (Flagged by: discussion — 1) - 🟡 Auto-open skips first artifact in new session (
useAutoOpenArtifacts.ts:31) — CodeRabbit reported; author acknowledged but deferred. (Flagged by: discussion — 1) - 🟡 Self-merged without human approval — PR merged by author with no explicit human APPROVED review. All reviews were COMMENTED state from bots.
🔎 QA session_id normalization all work correctly. Backend unit tests pass. Browser-based UI testing was blocked by auth middleware rejecting injected sessions (environment limitation, not a PR issue). @majdyz's automated E2E report shows 13/13 pass. Frontend unit test execution was attempted but environment setup prevented full validation.
🟠 Should Fix
useArtifactPanelneeds tests (useArtifactPanel.ts:89) — Copy handler has cached-vs-fetch branching,canCopygating for image/pdf, and Escape key handler. All testable viarenderHook. (testing — 1)ArtifactContentrenderer routing needs tests (ArtifactContent.tsx:76) — 8+ type branches with no direct coverage. A single test rendering each classification type would catch routing regressions. (testing — 1)- CSVRenderer test assertions need actual values (
CSVRenderer.test.ts:48-66) — Replacenot.toThrow()with assertions on parsed cell content. Verify quoted-newline and BOM-stripping actually work. (testing — 1) - AI-generated code network access (
reactArtifactPreview.ts:14-16) — Add CSPconnect-srcblocking private IP ranges (localhost, 10., 172.16-31., 192.168., 169.254.) or document in a threat model as accepted risk. (security — 1) - Arrow function component (
ChatContainer.tsx:37) — Convert to function declaration per codebase convention. (quality — 1) - Error handling via string matching (
routes.py:279-282) — IntroduceFileTooLargeError(ValueError)so the route usesisinstance()instead ofmessage.startswith(). (architect — 1) - CSVTable unbounded row rendering (
CSVRenderer.tsx:112) — Add a row cap (e.g., 500) with expansion, or use virtualization. A 100k-row CSV will freeze the browser. (discussion — 1)
🟡 Nice to Have
- Tailwind CDN SRI / self-hosting (
iframe-sandbox-csp.ts:32) — Self-host a vendored Tailwind build to eliminate CDN supply-chain risk. Complex due to JIT nature. (security) - Drag resize throttling (
ArtifactDragHandle.tsx:43) — UserequestAnimationFrameor CSS variables during drag to reduce React re-renders. (performance) - PDF blob caching (
useArtifactContent.ts:79) — Cache PDF blob URLs to avoid re-fetching on artifact switch. (performance) - Image error/loading state (
ArtifactContent.tsx:84) — AddonErrorhandler and skeleton to match other artifact types. (product) - Drag handle keyboard support (
ArtifactDragHandle.tsx:73) — Add arrow-key handler and ARIA value attributes for screen reader users. (product) - Detect 404 for deleted artifacts (
ArtifactContent.tsx:40) — Show "File deleted" instead of generic error with useless retry. (product) - Minimized strip close button (
ArtifactMinimizedStrip.tsx:21) — Add X button so users don't have to expand-then-close. (product)
🔵 Nits
- Duplicated width constants (
store.ts:64-65,ArtifactDragHandle.tsx:17-18) — Extract sharedMIN_PANEL_WIDTHandMAX_PANEL_WIDTH_PERCENTconstants. - Inline
MAX_HISTORY = 25(store.ts:205) — Move to module level alongsideDEFAULT_PANEL_WIDTH. - Exported props interface (
ChatContainer.tsx:14) —ChatContainerPropsis exported; convention says use unexportedPropsunless needed externally.
Human Review Needed
YES — This PR adds script-executing iframes rendering AI-generated content, modifies API routes with new file listing/upload endpoints, and was self-merged without explicit human approval. The security model (iframe sandboxing, CSP, SRI) and the network access from sandboxed code warrant human security review.
Risk Assessment
Merge risk: MEDIUM | Rollback: EASY (feature-flagged behind Flag.ARTIFACTS, defaults to false)
CI Status
❌ 2/6 local checks passed. Frontend lint, typecheck, build, and tests failed (environment setup issues — missing dependencies/config). Backend lint passed. Backend tests failed to run locally. Note: GitHub CI shows 47/50 checks passing with 3 non-blocking skips, suggesting local environment issues rather than code problems.
github-project-automation
Bot
moved this from ✅ Done
to 🚧 Needs work
in AutoGPT development kanban
There was a problem hiding this comment.
📋 Automated Review — PR #12629
PR #12629 — feat(platform): add copilot artifact preview panel
Author: ntindle | Files: 45
🎯 Verdict: REQUEST_CHANGES
PR Description Quality
✅ Has Why + What + How — PR describes the artifact preview panel feature, its renderers, backend support, and includes a visual demo.
What This PR Does
Adds a copilot artifact preview panel that lets users view AI-generated files (HTML, React, CSV, PDF, code, images) inline without leaving the chat. The panel supports resize/minimize/maximize, auto-opens on new artifacts, downloads, and renders each type with a dedicated previewer — including a sandboxed iframe for HTML/React with security isolation. Backend adds a list_workspace_files endpoint with pagination and file metadata tagging.
Specialist Findings
🛡️ Security allow-scripts without allow-same-origin), SRI hashes on React CDN, proper </script> escape, and thorough filename sanitization. However, two high-severity residual risks remain.
- 🟠 HTML/React iframes allow arbitrary outbound network requests (
ArtifactContent.tsx:119,reactArtifactPreview.ts:252) — AI-generated artifacts containing user PII can silently exfiltrate data via fetch/XHR. No CSPconnect-srcrestriction exists. This is documented as an accepted trade-off, but aconnect-srcallowlist for only the Tailwind CDN would close the gap without breaking functionality. (Flagged by: security, architect — 2) - 🟡 PDF iframe without sandbox (
ArtifactContent.tsx:98) — Intentionally omitted due to Chromium bug. Blob URL null origin mitigates most risk. Document more explicitly and re-test with newer Chromium. - 🟡
new Function()eval has no CPU/memory budget (reactArtifactPreview.ts:252) — A malicious React artifact could exhaust resources indefinitely. Consider an iframe kill-switch timeout.
🏗️ Architecture ✅ — Clean component structure following repo conventions (Component.tsx + useComponent.ts + helpers.ts), proper feature-flag gating, and good dependency direction with no circular refs.
- 🟠 Untyped
metadataparameter (workspace.py:158) —Optional[dict]accepts arbitrary structures. Define aTypedDictor Pydantic model to prevent drift as the metadata surface grows. (Flagged by: architect — 1) - 🟡 Renderer dispatch growing if/else chain (
ArtifactContent.tsx:66) — Consider a registry pattern (Record<ClassificationType, RenderFn>) to make adding new artifact types a registration rather than modification. - 🔵 Store growing beyond single-responsibility (
store.ts:121) —useCopilotUIStorenow manages 7+ concerns. Worth splitting into a dedicateduseArtifactStoreonce the feature stabilizes.
⚡ Performance ✅ — Adequate for expected usage patterns. Content cache bounded at 12 entries, artifact history at 25, fingerprint optimization limits unnecessary panel opens.
- 🟡 No transpile result caching (
transpileReactArtifact.ts:14) — TypeScripttranspileModulere-runs on back/forward navigation. A small Map cache keyed on source hash would eliminate redundant transpilation. - 🟡 PDF blob URLs not cached (
useArtifactContent.ts:79) — Each PDF view creates a new blob URL and fetch. Extend the content cache to cover blobs. - 🔵
useArtifactPanelsubscribes to entire sub-object (useArtifactPanel.ts:14) — Causes re-renders on every drag-resize event (~60Hz). Use granular selectors.
🧪 Testing
- 🟠 Backend soft-delete failure path untested (
routes.py:280) — Thetry/exceptaroundsoft_delete_workspace_filehas no test verifying 413 is returned when soft-delete raises. (Flagged by: testing — 1) - 🟠 Empty-string
session_idnormalization untested (routes.py:349) —session_id = session_id or Nonecoercion has no test sending?session_id=. (Flagged by: testing — 1) - 🟠 CSV render tests are smoke-only (
CSVRenderer.test.ts:49-67) — Only assertnot.toThrow(). Embedded-newline and escaped-quote cases should verify actual cell values. (Flagged by: testing — 1) - 🟠 Test isolation: content cache leaks between tests (
useArtifactContent.test.ts:39) — Module-levelcontentCacheis never cleared inbeforeEach. Tests use unique IDs as a workaround, but any future ID reuse produces false cache hits. (Flagged by: testing — 1)
📖 Quality ✅ — Readability score A. Excellent security documentation in iframe-sandbox-csp.ts and reactArtifactPreview.ts. Consistent naming, function declarations for components per repo conventions.
- 🟡 Duplicated HTML iframe logic (
ArtifactContent.tsx:113andHTMLRenderer.tsx:13) — Tailwind injection + sandbox + srcDoc repeated in two places. Extract a sharedHTMLPreviewIframecomponent. - 🔵
reactArtifactPreview.tsat 318 lines exceeds ~200-line frontend guideline. Bulk is a template literal with embedded runtime JS.
📦 Product
- 🟠 Image preview has no error state (
ArtifactContent.tsx:84) — IfsourceUrl404s, users see a broken image icon with no retry, unlike text/PDF paths which have explicit error handling. (Flagged by: product — 1) - 🟠 Desktop panel lacks focus management (
ArtifactPanel.tsx:99) — Opening/closing the panel doesn't move focus. Keyboard users are stranded. (Flagged by: product — 1) - 🟡 PDF iframe has no fallback (
ArtifactContent.tsx:98) — Browsers without inline PDF support show a blank panel. - 🟡 Drag handle invisible by default (
ArtifactDragHandle.tsx:83) — 1px transparent line only visible on hover. Users may not discover resize.
📬 Discussion
- 🟠
useAutoOpenArtifactsfirst-session bug acknowledged but unfixed (useAutoOpenArtifacts.ts:31) — Author said "Will investigate — tracking for follow-up" but no fix or tracking issue was created. First artifact in a new session may be skipped. (Flagged by: discussion, product — 2) - 🟠 Error handling via string matching (
routes.py:279) — 413 vs 409 determined bymessage.startswith('File too large'). Fragile — use custom exception subclasses. No author response. (Flagged by: discussion — 1) - 🟡 Internal storage paths exposed (
routes.py:375) —WorkspaceFileItem.pathleaks full internal path like/sessions/<session_id>/filename.txt.
🔎 QA pnpm generate:api not run in test env).
🟠 Should Fix
- Backend soft-delete failure path needs a test (
routes.py:280) — Add a test wheresoft_delete_workspace_fileraises and verify the response is still 413. (testing) - Empty-string
session_idnormalization needs a test (routes.py:349) — Send?session_id=and assert the manager receivessession_id=None. (testing) - CSV render tests need real assertions (
CSVRenderer.test.ts:49-67) — Verify parsed cell values for embedded-newline and escaped-quote cases, not justnot.toThrow(). (testing) - Clear content cache between tests (
useArtifactContent.test.ts:39) — AddclearContentCache()tobeforeEachto prevent inter-test pollution. (testing) - Image preview error handling (
ArtifactContent.tsx:84) — AddonErrorhandler with error state and retry/download button, consistent with text content error UI. (product) - Error string matching → custom exceptions (
routes.py:279) — Replacemessage.startswith('File too large')withFileTooLargeError/FileConflictErrorsubclasses. (discussion) useAutoOpenArtifactsfirst-session bug (useAutoOpenArtifacts.ts:31) — Fix the hydration detection so the first artifact after session reset auto-opens, or create a tracking issue. (discussion, product)
🟡 Nice to Have
- CSP
connect-srcon HTML/React iframes (ArtifactContent.tsx:119) — Allowlist only Tailwind CDN to close the exfiltration channel. Complex because it requires balancing live-preview functionality. (security, architect) - Type the
metadataparameter (workspace.py:158) —TypedDictwithorigin: Literal["user-upload", "agent-created"]. (architect) - Renderer registry pattern (
ArtifactContent.tsx:66) — Replace if/else chain withRecord<ClassificationType, RenderFn>for extensibility. (architect, quality) - Transpile result caching (
transpileReactArtifact.ts:14) — Small Map cache to avoid redundant TypeScript transpilation on back/forward navigation. (performance) - Focus management on panel open/close (
ArtifactPanel.tsx:99) — Move focus to close button on open, return to trigger on close. (product) - Extract shared
HTMLPreviewIframe(ArtifactContent.tsx:113,HTMLRenderer.tsx:13) — DRY up duplicated Tailwind+sandbox+srcDoc logic. (quality)
🔵 Nits
- Move
MAX_HISTORYto module scope (store.ts:205) — Currently inline insideopenArtifactaction body. - Document cache key uniqueness assumption (
useArtifactContent.ts:14) — Add one-line comment that artifact IDs are globally-unique UUIDs. formatSizeutility (ArtifactCard.tsx:16) — Consider moving to shared helpers for reuse.
QA Screenshots
| Screenshot | Description |
|---|---|
| N/A — browser auth blocked | Could not reach /copilot page due to Supabase middleware redirect. Backend API verified via direct HTTP. |
Human Review Needed
YES — Security-sensitive code (iframe sandboxing, new Function() eval, file storage), 45 files changed across frontend and backend, and frontend browser testing could not be completed. The iframe sandbox model and network exfiltration trade-off warrant human security review.
Risk Assessment
Merge risk: MEDIUM | Rollback: EASY (feature-flagged behind Flag.ARTIFACTS, disabled by default)
CI Status
❌ 4/6 local checks failed — Frontend lint, typecheck, build, and tests failed (missing generated API types). Backend lint passed ✅. Backend tests failed to start locally (Docker dependency) but 22 unit tests confirmed passing via pytest direct run.
| const wrapped = wrapWithHeadInjection(content, tailwindScript); | ||
| return ( | ||
| <iframe | ||
| sandbox="allow-scripts" |
There was a problem hiding this comment.
🤖 🟠 high (security/Data exfiltration via iframe)
HTML artifact iframe uses sandbox="allow-scripts" without CSP, allowing AI-generated HTML to make arbitrary fetch/XHR requests to external domains. If artifact content contains user PII from prompts, it can be silently exfiltrated by a malicious artifact.
Suggestion: Consider adding a connect-src CSP that allowlists only the Tailwind CDN and blocks all other outbound requests, or add a user-visible consent banner when HTML artifacts contain scripts that make network requests.
| whiteSpace: "pre-wrap", | ||
| }, | ||
| }, | ||
| this.state.error.stack || this.state.error.message || String(this.state.error), |
There was a problem hiding this comment.
🤖 🟠 high (security/Arbitrary code execution in iframe)
React preview uses new Function() to execute transpiled AI-generated code inside a sandboxed iframe. While isolated from the parent page, there is no CPU/memory budget — a malicious artifact could run crypto miners or exhaust resources indefinitely.
Suggestion: Consider adding a timeout mechanism (e.g., terminate the iframe after 30s of execution) or use a Web Worker with a kill switch for resource-intensive previews.
| // (Chromium bug #413851). The blob URL has a null origin so it can't | ||
| // access the parent page regardless. | ||
| return ( | ||
| <iframe src={pdfUrl} className="h-full w-full" title={artifact.title} /> |
There was a problem hiding this comment.
🤖 🟡 medium (security/Unsandboxed PDF iframe)
PDF iframe is rendered without any sandbox attribute. While the blob URL has a null origin, crafted PDFs with embedded JavaScript could potentially execute in the user's browser context depending on the PDF viewer implementation.
Suggestion: Document this risk more explicitly in a code comment, and consider testing whether sandbox="allow-scripts allow-same-origin" works with newer Chromium builds (the cited bug may be fixed).
| operation_id="listWorkspaceFiles", | ||
| ) | ||
| async def list_workspace_files( | ||
| user_id: Annotated[str, fastapi.Security(get_user_id)], |
There was a problem hiding this comment.
🤖 🟡 medium (security/File enumeration surface)
The new GET /files endpoint allows listing up to 1000 files per request with no rate limiting. When session_id is omitted, all workspace files across sessions are returned, enabling full workspace enumeration.
Suggestion: Consider adding rate limiting to this endpoint, or requiring session_id to be provided (making cross-session listing an explicit admin action).
| <div className="flex items-center justify-center p-4"> | ||
| {/* eslint-disable-next-line @next/next/no-img-element */} | ||
| <img | ||
| src={artifact.sourceUrl} |
There was a problem hiding this comment.
🤖 🟡 medium (security/Unvalidated sourceUrl at render site)
Image src is set directly from artifact.sourceUrl without validating it matches the expected /api/proxy/... pattern. If sourceUrl construction is ever changed or bypassed, this could become an SSRF or content injection vector.
Suggestion: Add a validation check that sourceUrl matches the expected proxy URL pattern before rendering it in an img/iframe src attribute.
| useEffect(() => { | ||
| messageFingerprintsRef.current = new Map(); | ||
| hasInitializedRef.current = false; | ||
| }, [sessionId]); |
There was a problem hiding this comment.
🤖 🟡 medium (discussion/Unresolved bug)
Author acknowledged that the first artifact in a new session may be skipped (treated as hydration instead of live). CodeRabbit flagged as Major, author said 'Will investigate — tracking for follow-up' but no fix was committed.
Suggestion: Fix the hydration detection logic so that after a sessionId reset, the first assistant artifact is recognized as live and auto-opened. At minimum, create a tracking issue.
| except ValueError as e: | ||
| raise fastapi.HTTPException(status_code=409, detail=str(e)) from e | ||
| # write_file raises ValueError for both path-conflict and size-limit | ||
| # cases; map each to its correct HTTP status. |
There was a problem hiding this comment.
🤖 🟡 medium (discussion/Error handling fragility)
HTTP status 413 vs 409 is determined by message.startswith('File too large'). If the error message text changes in WorkspaceManager, the wrong status code will be returned silently. No reviewer response from author.
Suggestion: Use a custom exception subclass (e.g., FileTooLargeError vs FileConflictError) instead of string matching on ValueError messages.
| ) | ||
| has_more = len(files) > limit | ||
| page = files[:limit] | ||
|
|
There was a problem hiding this comment.
🤖 🟢 low (discussion/Information exposure)
WorkspaceFileItem exposes the full internal path (e.g., /sessions/<session_id>/filename.txt) to the frontend API response. This leaks session-scoped internal storage structure.
Suggestion: Consider returning only the filename or a sanitized relative path instead of the full internal storage path.
| @@ -0,0 +1,125 @@ | |||
| "use client"; | |||
There was a problem hiding this comment.
🤖 🟢 low (discussion/Test coverage)
Frontend patch coverage is 38.72%, well below the 80% target. @0ubbe suggested component-level integration tests instead of isolated hook unit tests, which was not addressed.
Suggestion: Add component-level integration tests that render ArtifactPanel with mocked API responses, as suggested by @0ubbe.
| "React", | ||
| "ReactDOM", | ||
| "module", | ||
| "exports", |
There was a problem hiding this comment.
🤖 🟢 low (discussion/Stale follow-up)
Author acknowledged SRI hashes for React CDN bundles would be reasonable to add and said 'Will track for follow-up' — SRI hashes ARE present in the final code (sha384-), so this was actually addressed. However, Tailwind CDN at line 296 remains unpinned.
Suggestion: Consider using a pinned Tailwind CDN version URL (e.g., cdn.tailwindcss.com/3.4.1) even if SRI isn't practical for JIT output.
There was a problem hiding this comment.
❓ INCONCLUSIVE
Review synthesis complete.
Risk level: medium | Duration: 88s | Reviewed: b732d10e
Specialist Reports
| Specialist | Status | Summary |
|---|---|---|
| security | ❌ FAIL | security review complete. 0 finding(s). |
| architect | ❌ FAIL | architect review complete. 0 finding(s). |
| performance | ❌ FAIL | performance review complete. 0 finding(s). |
| testing | ❌ FAIL | testing review complete. 0 finding(s). |
| quality | ❌ FAIL | quality review complete. 0 finding(s). |
| product | ❌ FAIL | product review complete. 0 finding(s). |
| discussion | ❌ FAIL | discussion review complete. 0 finding(s). |
| ui-reviewer | ❌ FAIL | ui-reviewer review complete. 0 finding(s). |
Quality Checks
- ❌ lint: cd autogpt_platform/frontend && pnpm lint:
cd autogpt_platform/frontend && pnpm lint(1s) - ✅ lint: cd autogpt_platform/backend && poetry run lint:
cd autogpt_platform/backend && poetry run lint(65s) - ❌ typecheck: cd autogpt_platform/frontend && pnpm types:
cd autogpt_platform/frontend && pnpm types(0s) - ❌ test: cd autogpt_platform/backend && poetry run test:
cd autogpt_platform/backend && poetry run test(0s) - ❌ test: cd autogpt_platform/frontend && pnpm test:unit:
cd autogpt_platform/frontend && pnpm test:unit(0s) - ❌ build: cd autogpt_platform/frontend && pnpm build:
cd autogpt_platform/frontend && pnpm build(0s)
There was a problem hiding this comment.
❓ INCONCLUSIVE
Review synthesis complete.
Risk level: medium | Duration: 82s | Reviewed: b732d10e
Specialist Reports
| Specialist | Status | Summary |
|---|---|---|
| security | ❌ FAIL | security review complete. 0 finding(s). |
| architect | ❌ FAIL | architect review complete. 0 finding(s). |
| performance | ❌ FAIL | performance review complete. 0 finding(s). |
| testing | ❌ FAIL | testing review complete. 0 finding(s). |
| quality | ❌ FAIL | quality review complete. 0 finding(s). |
| product | ❌ FAIL | product review complete. 0 finding(s). |
| discussion | ❌ FAIL | discussion review complete. 0 finding(s). |
| ui-reviewer | ❌ FAIL | ui-reviewer review complete. 0 finding(s). |
Quality Checks
- ❌ lint: cd autogpt_platform/frontend && pnpm lint:
cd autogpt_platform/frontend && pnpm lint(1s) - ✅ lint: cd autogpt_platform/backend && poetry run lint:
cd autogpt_platform/backend && poetry run lint(63s) - ❌ typecheck: cd autogpt_platform/frontend && pnpm types:
cd autogpt_platform/frontend && pnpm types(0s) - ❌ test: cd autogpt_platform/backend && poetry run test:
cd autogpt_platform/backend && poetry run test(0s) - ❌ test: cd autogpt_platform/frontend && pnpm test:unit:
cd autogpt_platform/frontend && pnpm test:unit(0s) - ❌ build: cd autogpt_platform/frontend && pnpm build:
cd autogpt_platform/frontend && pnpm build(0s)
There was a problem hiding this comment.
📋 Automated Review — PR #12629
PR #12629 — feat(platform): add copilot artifact preview panel
Author: ntindle | Files: 45
🎯 Verdict: REQUEST_CHANGES
PR Description Quality
What This PR Does
Adds a dedicated artifact preview panel to the copilot chat interface. When the AI assistant creates or references workspace files (HTML, PDF, CSV, React components, code, images), they're automatically classified and rendered in a resizable side panel with type-appropriate previews. The backend gains a new paginated file listing endpoint (GET /files), origin metadata tracking on uploads, and proper 409/413 error code mapping. The entire feature is gated behind a Flag.ARTIFACTS feature flag (default off).
Specialist Findings
🛡️ Security sandbox="allow-scripts" without allow-same-origin), with SRI on React/ReactDOM and proper </script> escaping. However, sandboxed iframes permit unrestricted outbound network requests, enabling browser-side SSRF from AI-generated HTML/React artifacts. PDF iframe has no sandbox attribute due to a Chromium bug.
- 🟠 HTML/React artifact iframes allow arbitrary
fetch()/XHR/WebSocketto internal network targets (ArtifactContent.tsx:119,reactArtifactPreview.ts:259) — browser-SSRF risk. (Flagged by: security, architect — 2) - 🟡 PDF iframe unsandboxed due to Chromium bug #413851 (
ArtifactContent.tsx:98) — blob URL opaque origin mitigates, but defense-in-depth gap. - 🟡 Tailwind CDN loaded without SRI (
iframe-sandbox-csp.ts:33) — JIT nature makes SRI impractical; sandbox limits blast radius. (Flagged by: security, architect, discussion — 3) - 🟢 Download filename sanitization, upload path traversal prevention,
</script>escaping, React SRI hashes, and feature flag gating are all solid.
🏗️ Architecture ✅ — Clean component/hook/helper separation following project conventions. Feature flag discipline is thorough. Dynamic import for the panel keeps the critical path lean. Zustand store integration is well-typed.
- 🟡
reactArtifactPreview.tsembeds a 270-line JS runtime as a template literal (reactArtifactPreview.ts:49) — maintainability concern. Extract to a separate file. - 🟡 Store imports
clearContentCachefrom a deeply nested component path (store.ts:3), inverting the expected dependency direction. - 🔵
WorkspaceFileItemmanually duplicates fields fromWorkspaceFile(routes.py:134) — consider Pydantic inheritance.
⚡ Performance ✅ — Proper LRU caching (12 entries), bounded history (25 entries), efficient limit+1 pagination pattern, good cancellation via AbortController.
- 🟠 TypeScript compiler (~3-5MB) loaded and runs
transpileModuleon the main thread (transpileReactArtifact.ts:14) — blocks UI during React artifact transpilation. Web Worker would prevent jank. - 🟠 PDF blobs re-fetched on every view (
useArtifactContent.ts:79) — no caching unlike text content. Multi-MB re-downloads when toggling between artifacts. - 🟡
extractWorkspaceArtifactscreates new RegExp objects per URI match (ChatMessagesContainer/helpers.ts:272) — O(n) regex compilations during streaming.
🧪 Testing
- 🟠
useArtifactPanelhook has zero tests (useArtifactPanel.ts:13) — keyboard escape handling, copy-from-cache, width clamping, and maximize logic are all untested. (Flagged by: testing — 1) - 🟠
ArtifactContent/ArtifactRendererrouting has zero tests (ArtifactContent.tsx:78) — the switch-chain selecting between 8+ renderer types is the most regression-prone code and is completely uncovered. (Flagged by: testing — 1) - 🟠 CSV smoke tests only assert
not.toThrow()(CSVRenderer.test.ts:49) — don't verify parsed output correctness. A parser silently dropping rows would pass. - 🟡
ArtifactDragHandlepointer event math untested. Backendworkspace_files.pymetadata={"origin": "agent-created"}passthrough untested. - 🟡 Frontend patch coverage at 38.72% vs 80% target — significant gap noted in CI.
📖 Quality ✅ — Excellent security documentation (iframe-sandbox-csp.ts:1-29, reactArtifactPreview.ts:1-19). Clean naming, consistent formatting, proper interface Props pattern.
- 🟡 HTML iframe rendering logic duplicated between
ArtifactContent.tsx:113andHTMLRenderer.tsx:13— extract a sharedSandboxedHTMLPreview. - 🔵 Min panel width
320duplicated instore.ts:65andArtifactDragHandle.tsx:18— share a constant. - 🔵
session_id = session_id or Noneduplicated inroutes.py:218androutes.py:364.
📦 Product ✅ — Feature-complete against stated goals. Auto-open, history navigation, classification pipeline, resize/minimize/maximize, mobile sheet overlay all implemented. Feature flag gating provides safe rollout.
- 🟡 Image preview has no error/loading fallback (
ArtifactContent.tsx:82) — broken URLs show browser broken-image icon. - 🟡 Files >10MB silently classified as download-only with no user hint (
helpers.ts:203). - 🔵 Copy button on HTML artifacts copies raw source, not visible rendered text (
useArtifactPanel.ts:89).
📬 Discussion
- 🟠
canCopychecksclassification.label(display string) instead ofclassification.type(stable enum) (useArtifactPanel.ts:112) — cursor[bot] flagged, no author response. Label rename silently breaks copy guard. - 🟡
closeArtifactPanelnullifiesactiveArtifactbefore exit animation completes (ArtifactPanel.tsx:100) — cursor[bot] flagged, no response. - 🟡 First artifact in new sessions skipped by auto-open — author acknowledged, deferred to follow-up.
🔎 QA ✅ — Live API testing confirmed all backend endpoints (list/upload/download/delete/storage) with correct status codes, pagination, metadata tracking, and error handling. 948 frontend + 22 backend tests pass. Copilot page loads correctly. Build page shows no regression.
🟠 Should Fix
useArtifactPanelneeds tests (useArtifactPanel.ts:13) — Zero coverage on the main orchestration hook: escape key handling, copy-from-cache path, effective width clamping, maximize logic. This is the most regression-prone hook in the feature. (Flagged by: testing — 1)ArtifactContentrouting needs tests (ArtifactContent.tsx:78) — The switch-chain dispatching to 8+ renderer types has zero coverage. A regression (e.g., code rendering through MarkdownRenderer) would be invisible. (Flagged by: testing — 1)canCopyshould checkclassification.typenotclassification.label(useArtifactPanel.ts:112) — Using a display string as a logic gate is fragile. If the label "PDF" is ever renamed, the copy guard breaks silently. (Flagged by: discussion — 1)- CSV smoke tests need correctness assertions (
CSVRenderer.test.ts:49) — Currently only assertnot.toThrow(). Add assertions verifying row count, column alignment, and quoted-field handling. (Flagged by: testing — 1)
🟡 Nice to Have
- Move TypeScript transpilation to a Web Worker (
transpileReactArtifact.ts:14) — The ~3-5MB TS compiler running on the main thread blocks UI. ApostMessagewrapper would keep the UI responsive. (performance) - Add PDF blob caching (
useArtifactContent.ts:79) — Unlike text content with a 12-entry LRU, PDFs re-fetch on every view. Add a small blob URL cache. (performance) - Extract iframe runtime to separate file (
reactArtifactPreview.ts:49) — 270-line JS template literal is hard to maintain/test. (architect, quality) - Image error fallback (
ArtifactContent.tsx:82) — AddonErrorhandler with retry button for broken image URLs. (product) - Add CSP
connect-srcrestriction for simple HTML artifacts (ArtifactContent.tsx:119) — Mitigates browser-SSRF risk from AI-generated content. (security) - Invert store→cache dependency (
store.ts:3) — MoveclearContentCacheto a shared utility instead of importing from component internals. (architect)
🔵 Nits
- Share min-width constant (
ArtifactDragHandle.tsx:18,store.ts:65) —320appears in two places. - DRY session_id normalization (
routes.py:218,routes.py:364) — Extract a tiny helper. - Add
aria-labelfor vertical text (ArtifactMinimizedStrip.tsx:37) —writing-mode: vertical-rlmay not announce well with screen readers.
QA Screenshots
| Screenshot | Description |
|---|---|
 |
Copilot page loads correctly with chat interface ✅ |
 |
Chat UI functional with input, sidebar, suggestions ✅ |
 |
Build page renders correctly — no regression ✅ |
Human Review Needed
YES — This is a 45-file feature PR introducing client-side code execution (iframe sandboxing model), new backend API endpoints, and a security-sensitive artifact rendering pipeline. The iframe sandbox decisions and outbound network access trade-offs warrant human security review.
Risk Assessment
Merge risk: MEDIUM | Rollback: EASY (feature flag Flag.ARTIFACTS defaults to false)
CI Status
❌ 2/6 quality checks passed. Frontend lint, typecheck, unit tests, build, and backend tests showed failures in the automated harness (some appear to be environment issues — the live test run showed 948 frontend tests and 22 backend tests passing). Backend lint passes.
| const wrapped = wrapWithHeadInjection(content, tailwindScript); | ||
| return ( | ||
| <iframe | ||
| sandbox="allow-scripts" |
There was a problem hiding this comment.
🤖 🟠 high (security/Browser SSRF via sandboxed iframe)
HTML artifact iframe uses sandbox="allow-scripts" without CSP connect-src restriction. AI-generated HTML can make arbitrary fetch/XHR/WebSocket requests from the user's browser, enabling SSRF against internal network services or exfiltration of artifact content.
Suggestion: Consider a tiered approach: add connect-src 'none' CSP for simple HTML artifacts, and only relax it for artifacts explicitly classified as dashboards/charts. Alternatively, document this as an accepted risk in the threat model.
| return this.props.children; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🤖 🟠 high (security/Code execution + network access in React preview)
React preview uses new Function() to execute transpiled AI-generated code inside a sandboxed iframe. Combined with unrestricted outbound network access, a malicious artifact could make requests to internal services from the user's browser.
Suggestion: Ensure sandbox="allow-scripts" is never weakened to include allow-same-origin. Consider adding a comment-level invariant or test that asserts the sandbox attribute value. The current implementation is safe but fragile if modified.
| // (Chromium bug #413851). The blob URL has a null origin so it can't | ||
| // access the parent page regardless. | ||
| return ( | ||
| <iframe src={pdfUrl} className="h-full w-full" title={artifact.title} /> |
There was a problem hiding this comment.
🤖 🟡 medium (security/Unsandboxed PDF iframe)
PDF iframe has no sandbox attribute due to Chromium bug #413851. While the blob URL provides an opaque origin, PDFs with embedded JavaScript actions could execute in the unsandboxed context.
Suggestion: Add a comment documenting this as an accepted risk, or consider using a JS-based PDF renderer (like pdf.js) that can render within a sandboxed iframe.
|
|
||
| // Pinned to a specific version to reduce exposure to unannounced upstream | ||
| // changes (SRI is not possible because the JIT runtime is generated on demand). | ||
| export const TAILWIND_CDN_URL = "https://cdn.tailwindcss.com/3.4.16"; |
There was a problem hiding this comment.
🤖 🟡 medium (security/No SRI for Tailwind CDN)
Tailwind CDN is loaded without SRI integrity hash in both HTML and React preview iframes. A CDN compromise would allow arbitrary JS execution inside artifact previews.
Suggestion: The comment acknowledges SRI isn't possible for Tailwind's JIT runtime. Consider self-hosting the Tailwind build to eliminate CDN dependency, or pin to a non-JIT build that supports SRI.
| <div className="flex items-center justify-center p-4"> | ||
| {/* eslint-disable-next-line @next/next/no-img-element */} | ||
| <img | ||
| src={artifact.sourceUrl} |
There was a problem hiding this comment.
🤖 🟡 medium (security/Unvalidated image src URL)
Image src is set directly from artifact.sourceUrl without validating it matches the expected /api/proxy/... pattern. If the URL construction in extractWorkspaceArtifacts is bypassed or manipulated, arbitrary URLs could be loaded.
Suggestion: Add a URL allowlist check (e.g., must start with '/api/proxy/' or be a relative path) before setting the img src.
| * React is loaded from unpkg with pinned version and SRI integrity hashes. | ||
| */ | ||
|
|
||
| import { TAILWIND_CDN_URL } from "@/lib/iframe-sandbox-csp"; |
There was a problem hiding this comment.
🤖 🟡 medium (discussion/Unresolved review feedback)
Tailwind CDN loaded via unpinned https://cdn.tailwindcss.com with no SRI hash. While sandbox isolates the iframe, a CDN compromise would inject arbitrary JS into every React/HTML artifact preview. Reviewer @majdyz flagged this as a blocker; author dismissed citing JIT nature.
Suggestion: Consider pinning the Tailwind Play CDN to a specific version URL (e.g., https://cdn.tailwindcss.com/3.4.17) even if SRI isn't practical for the JIT compiler, to prevent silent breakage from upstream updates.
| variant: "destructive", | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🤖 🟢 low (discussion/Unacknowledged review feedback)
cursor[bot] flagged that canCopy checks classification.label !== 'PDF' (display string) instead of classification.type !== 'pdf' (stable enum). No response from author. Label changes would silently break the copy guard.
Suggestion: Switch the canCopy check to use classification.type instead of classification.label for robustness.
| } | ||
|
|
||
| // Keep AnimatePresence mounted across the open→closed transition so the | ||
| // exit animation on the motion.div has a chance to run. |
There was a problem hiding this comment.
🤖 🟢 low (discussion/Unacknowledged review feedback)
cursor[bot] flagged that closeArtifactPanel nullifies activeArtifact in the same state update as setting isOpen=false, causing the if (!activeArtifact) return null guard to unmount the component before AnimatePresence exit animation can play. No response from author.
Suggestion: Defer clearing activeArtifact until after the exit animation completes, or keep activeArtifact populated while isOpen transitions to false.
| <th | ||
| key={i} | ||
| className="px-3 py-2 text-left font-medium text-zinc-700" | ||
| > |
There was a problem hiding this comment.
🤖 🟢 low (discussion/Accessibility)
coderabbitai flagged that column sorting click handlers on <th> elements are not keyboard-accessible. No response from author.
Suggestion: Use a <button> inside the <th> or add tabIndex={0} and onKeyDown handler for Enter/Space.
| useEffect(() => { | ||
| messageFingerprintsRef.current = new Map(); | ||
| hasInitializedRef.current = false; | ||
| }, [sessionId]); |
There was a problem hiding this comment.
🤖 🟡 medium (discussion/Deferred bug)
coderabbitai identified that the first live artifact in a brand-new session is skipped by the auto-open logic because hasInitializedRef is false after sessionId reset. Author acknowledged and deferred to follow-up.
Suggestion: Track this as a follow-up issue to ensure it doesn't get lost — it affects the core happy path of the feature.
There was a problem hiding this comment.
❓ INCONCLUSIVE
You've hit your limit · resets 7pm (UTC)
Risk level: medium | Duration: 238s | Reviewed: b732d10e
Specialist Reports
| Specialist | Status | Summary |
|---|---|---|
| security | ✅ PASS | Sandbox model is sound but HTML/React iframes allow outbound network from AI-generated content; PDF iframe lacks sandbox; file path exposed in list response. |
| architect | ✅ PASS | Well-structured artifact preview system with proper feature flag gating, clean separation of concerns, and solid security model; a few architectural improvements around caching and file length would help maintainability. |
| performance | ✅ PASS | Performance is well-considered with proper caching, size gates, and cleanup; a few optimization opportunities exist for React transpilation caching, CSV rendering of large files, and PDF blob caching. |
| testing | Good helper/store test coverage, but useArtifactPanel hook, ArtifactContent renderer routing, and ArtifactDragHandle have zero tests despite containing critical business logic. | |
| quality | ✅ PASS | Well-structured, well-documented PR with excellent security rationale; minor improvements needed for shared constants and one oversized file. |
| product | ✅ PASS | Well-implemented artifact preview panel with comprehensive type support, proper feature flagging, and good error handling; has minor accessibility gaps in keyboard resize, focus indicators, and screen reader annotations that should be addressed in follow-up. |
| discussion | All CI green and feature is flagged, but CSP security hardening was reverted without discussion, @0ubbe's testing feedback is unacknowledged, and frontend coverage is 38% vs 80% target. | |
| ui-reviewer | ✅ PASS | I'll start by reading my tips/credentials files and understanding the environment, then test the actual feature. Good — frontend (200) and backend (200) are healthy. Let me set up auth, test the backend API endpoints, and open the browser. ``` |
Findings: 🔴 0 critical | 🟠 3 high | 🟡 15 medium | 🟢 25 low
Blockers
- 🟠
autogpt_platform/frontend/src/app/(platform)/copilot/components/ArtifactPanel/useArtifactPanel.ts:13useArtifactPanel has no tests. It contains handleCopy (with cache-then-fetch fallback), handleDownload, Escape key handling with dialog conflict check, viewport resize throttling, and canCopy derivation — all untested.
Suggestion: Add a test file useArtifactPanel.test.ts covering: (1) handleCopy uses cached content when available, (2) handleCopy falls back to fetch when cache misses, (3) handleCopy error path shows toast, (4) canCopy is false for image/pdf/download-only, (5) Escape key closes panel, (6) Escape key is suppressed when a dialog is open. - 🟠
autogpt_platform/frontend/src/app/(platform)/copilot/components/ArtifactPanel/components/ArtifactContent.tsx:76ArtifactRenderer has ~10 conditional rendering branches (image, PDF, HTML with Tailwind injection, React preview, code, JSON, CSV, registry fallback, plain text) — none are tested. This is the core rendering pipeline of the feature.
Suggestion: Add integration tests for ArtifactRenderer (or ArtifactContent) that verify the correct renderer is selected for each classification type, using shallow rendering or snapshot tests. - 🟠
autogpt_platform/frontend/src/app/(platform)/copilot/components/ArtifactPanel/components/ArtifactContent.tsx:132HTML artifact iframe uses sandbox='allow-scripts' without CSP. The CSP meta tag was added in 48354ac per @majdyz's security review, then removed in 311b26d without discussion. Scripts inside can make arbitrary network requests, enabling potential exfiltration of AI-generated content.
Suggestion: Re-add CSP meta tag to HTML previews or document in the PR why it was removed and what mitigations replace it.
Should Fix
- 🟡
autogpt_platform/frontend/src/app/(platform)/copilot/components/ArtifactPanel/components/ArtifactContent.tsx:119HTML artifacts rendered with sandbox='allow-scripts' can make arbitrary outbound fetch/XHR requests to any origin. AI-generated or manipulated HTML could probe internal network hosts or exfiltrate rendered content.
Suggestion: Consider adding connect-src restrictions via meta CSP for artifacts that don't need network access, or document the accepted risk in a security decision record. - 🟡
autogpt_platform/frontend/src/app/(platform)/copilot/components/ArtifactPanel/components/reactArtifactPreview.ts:263AI-generated TSX is transpiled and executed via new Function() in a sandboxed iframe. The custom require() shim only allows react/react-dom but compiled code can still make outbound network requests within the sandbox.
Suggestion: Acceptable given sandbox isolation. Consider rate-limiting or logging artifact previews to detect abuse patterns. - 🟡
autogpt_platform/frontend/src/app/(platform)/copilot/components/ArtifactPanel/components/ArtifactContent.tsx:98PDF preview iframe has no sandbox attribute. While the blob: URL gets a null origin, the lack of any sandbox means if the blob origin model changes in a future browser, scripts in a crafted PDF could access the parent.
Suggestion: Add sandbox='' (empty — blocks all capabilities) to the PDF iframe. Chrome's PDF viewer doesn't need allow-scripts. - 🟡
autogpt_platform/backend/backend/api/features/workspace/routes.py:381WorkspaceFileItem exposes the full path field which contains session IDs (e.g. /sessions/sess-123/file.txt), leaking session structure to the frontend.
Suggestion: Consider omitting the path field from the response or stripping the session prefix if the frontend doesn't need it. - 🟢
autogpt_platform/frontend/src/lib/iframe-sandbox-csp.ts:33Tailwind CDN script loaded without SRI integrity hash. A CDN compromise would affect all HTML/React artifact previews. React/ReactDOM correctly use SRI.
Suggestion: Document this as an accepted risk. Consider self-hosting the Tailwind CDN script to eliminate the external dependency. - 🟢
autogpt_platform/frontend/src/app/(platform)/copilot/components/ArtifactPanel/components/ArtifactContent.tsx:41Raw fetch error messages are displayed to the user, which could reveal internal API URLs or infrastructure details.
Suggestion: Use generic user-facing error messages and log the raw error to the console/Sentry instead. - 🟡
autogpt_platform/frontend/src/app/(platform)/copilot/components/ArtifactPanel/components/useArtifactContent.ts:14Module-level Map used as a manual LRU cache for artifact content. This bypasses the project's standard React Query/TanStack Query pattern for server state, creating hidden global state that isn't cleared on session switches and survives hot reloads.
Suggestion: Replace with a useQuery hook using staleTime/gcTime for automatic cache lifecycle, deduplication, and proper React integration. This eliminates the manual LRU logic and aligns with the project's data-fetching conventions. - 🟢
autogpt_platform/frontend/src/app/(platform)/copilot/components/ChatContainer/ChatContainer.tsx:37ChatContainer is exported as an arrow function (export const ChatContainer = ({...}) => {), violating the project convention of using function declarations for components.
Suggestion: Change toexport function ChatContainer({...}: ChatContainerProps) {. - 🟢
autogpt_platform/frontend/src/app/(platform)/copilot/components/ArtifactPanel/components/reactArtifactPreview.ts:1At 318 lines, this file exceeds the project's ~200 line frontend file guideline. The inline 225-line JavaScript runtime template embedded as a string is the primary contributor and is difficult to audit/test in isolation.
Suggestion: Extract the runtime template into a separate file (e.g., reactArtifactRuntime.ts) and keep this file as the public API surface. - 🟢
autogpt_platform/frontend/src/app/(platform)/copilot/components/ArtifactPanel/components/ArtifactContent.tsx:4Direct import ofcodeRendererbypasses the globalRegistry pattern used for CSV and JSON rendering, creating inconsistent coupling.
Suggestion: Use globalRegistry.getRenderer(content, codeMeta) with explicit type metadata, matching the pattern used for JSON and CSV in the same file. - 🟢
autogpt_platform/backend/backend/copilot/tools/workspace_files.py:848Origin metadata strings ('agent-created', 'user-upload') are hardcoded in multiple locations across backend routes and copilot tools with no shared constant.
Suggestion: Define shared constants (e.g., ORIGIN_USER_UPLOAD, ORIGIN_AGENT_CREATED) in a common module to prevent string drift. - 🟢
autogpt_platform/frontend/src/app/(platform)/copilot/components/ArtifactPanel/components/ArtifactContent.tsx:66ArtifactRenderer function is ~120 lines with an inline props type, exceeding the ~50 line component guideline. The cascading if-chain for each artifact type could be extracted.
Suggestion: Extract a renderByType helper or use a type→renderer map to reduce the function body, and name the inline props as a proper interface. - 🟡
autogpt_platform/frontend/src/app/(platform)/copilot/components/ArtifactPanel/components/transpileReactArtifact.ts:14Full TypeScript compiler (~3-5MB) is dynamically imported on every React artifact preview. Concurrent rapid artifact switches could trigger parallel loads.
Suggestion: Cache the import promise in a module-level variable:let tsPromise: Promise<typeof import('typescript')> | null = null;and reuse it across calls. - 🟡
autogpt_platform/frontend/src/components/contextual/OutputRenderers/renderers/CSVRenderer.tsx:22Character-by-character CSV parser with string concatenation (current += ch) creates GC pressure on large CSVs (up to 10MB per the size gate). The table also renders all rows into the DOM at once.
Suggestion: Consider using an array-based accumulator (chars.push(ch); chars.join('')) for the parser, and add row-count virtualization or a cap (e.g., first 500 rows with a 'show more' button) for the table render. - 🟢
autogpt_platform/frontend/src/app/(platform)/copilot/components/ChatMessagesContainer/helpers.ts:272Two new RegExp objects are compiled per workspace URI match inside extractWorkspaceArtifacts. With many workspace URIs in a message, this adds up.
Suggestion: Pre-compile a single regex outside the loop or useString.includes()/String.indexOf()for the image-link check instead of creating a new RegExp per URI. - 🟢
autogpt_platform/frontend/src/app/(platform)/copilot/components/ArtifactPanel/components/useArtifactContent.ts:79PDF artifacts are re-fetched and a new blob URL created every time the user navigates back to a previously viewed PDF, unlike text artifacts which benefit from the contentCache.
Suggestion: Add a small cache (e.g., Map<id, blobUrl>) for PDF blob URLs with proper cleanup on eviction, similar to the text content cache. - 🟢
autogpt_platform/frontend/src/app/(platform)/copilot/components/ArtifactPanel/components/reactArtifactPreview.ts:97Three CDN scripts (Tailwind, React, ReactDOM) in are render-blocking. On slow connections the preview appears blank while all three download sequentially.
Suggestion: Move the Tailwind CDN <script> after the React scripts or usedeferso it doesn't block React initialization. React/ReactDOM must remain synchronous since the inline runtime depends on them. - 🟢
autogpt_platform/frontend/src/app/(platform)/copilot/components/ChatContainer/useAutoOpenArtifacts.ts:75A new Set is allocated from all message IDs on every streaming chunk to clean up stale fingerprints. During active streaming this fires on every token.
Suggestion: Only rebuild the liveIds set when the message count changes (trackmessages.lengthin a ref), or move the cleanup to a less frequent path (e.g., on session switch). - 🟢
autogpt_platform/backend/backend/api/features/workspace/routes.py:348Default limit=200 (max 1000) for list_workspace_files is generous for a frontend that likely displays a scrollable list. Large responses increase serialization time and payload size.
Suggestion: Consider reducing the default to 50 and the max to 200, which should cover most UI use cases while keeping responses lean. - 🟡
autogpt_platform/frontend/src/app/(platform)/copilot/components/ArtifactPanel/components/ArtifactDragHandle.tsx:1ArtifactDragHandle — the resize behavior and the fix for 'maximized-pane resize path so dragging exits maximized mode immediately' (a stated PR goal) have no tests.
Suggestion: Add tests verifying: (1) pointer drag calls onWidthChange with clamped values, (2) dragging respects minWidth and maxWidthPercent bounds. - 🟡
autogpt_platform/frontend/src/components/contextual/OutputRenderers/renderers/CSVRenderer.test.ts:52CSV render smoke tests only assertnot.toThrow()— they never verify the parsed output. A parser bug that silently drops rows or corrupts cell data would pass these tests.
Suggestion: For at least the 'embedded newline inside quoted field' case, assert the rendered table has the correct number of rows/cells, e.g. by checking the returned React element tree or rendered DOM. - 🟡
autogpt_platform/backend/backend/api/features/workspace/routes.py:363list_workspace_files normalizes empty-string session_id to None (line 363), but no test verifies GET /files?session_id= behaves the same as GET /files (include_all_sessions=True).
Suggestion: Add a test: test_list_files_empty_session_id_treated_as_none that calls GET /files?session_id= and asserts include_all_sessions=True is passed to list_files. - 🟡
autogpt_platform/frontend/src/app/(platform)/copilot/components/ArtifactCard/ArtifactCard.tsx:1ArtifactCard has no tests. It has two distinct code paths: openable artifacts (click-to-open) vs download-only artifacts (click-to-download). The active state styling toggle is also untested.
Suggestion: Add tests verifying: (1) openable artifact calls openArtifact on click, (2) non-openable artifact calls downloadArtifact on click, (3) active state renders correct CSS class. - 🟢
autogpt_platform/backend/backend/copilot/tools/workspace_files.py:848The new metadata={'origin': 'agent-created'} parameter passed on agent file creation has no test coverage verifying it reaches write_file.
Suggestion: Add a unit test for the copilot workspace file tool that asserts metadata={'origin': 'agent-created'} is passed to write_file. - 🟢
autogpt_platform/frontend/src/app/(platform)/copilot/components/ArtifactPanel/components/ArtifactDragHandle.tsx:18maxWidthPercent default (85) is duplicated in store.ts:64 (0.85) and useArtifactPanel.ts:127 (0.85). Drift between these will cause inconsistent behavior.
Suggestion: Extract a sharedMAX_PANEL_WIDTH_PERCENT = 85constant in store.ts and import it in ArtifactDragHandle and useArtifactPanel. - 🟢
autogpt_platform/frontend/src/app/(platform)/copilot/store.ts:205MAX_HISTORY = 25 is defined inline inside the openArtifact action body rather than as a module-level constant.
Suggestion: Moveconst MAX_HISTORY = 25to module scope alongside DEFAULT_PANEL_WIDTH for visibility and reuse. - 🟢
autogpt_platform/frontend/src/app/(platform)/copilot/components/ArtifactPanel/components/reactArtifactPreview.ts:1At 318 lines this file exceeds the frontend guideline of ~200 lines. The bulk is the HTML template string in buildReactArtifactSrcDoc.
Suggestion: Consider extracting the runtime JS or the full HTML template into a separate constant file (e.g., reactArtifactTemplate.ts) to keep each file focused. - 🟢
autogpt_platform/frontend/src/app/(platform)/copilot/components/ArtifactPanel/components/ArtifactContent.tsx:66ArtifactRenderer has a cascading if-chain (~120 lines) for each artifact type. Adding new types requires modifying this growing chain.
Suggestion: Consider a lookup map (type → render function) to make the dispatch more declarative and easier to extend. - 🟢
autogpt_platform/frontend/src/app/(platform)/copilot/components/ArtifactPanel/components/ArtifactMinimizedStrip.tsx:37Uses inline style={{writingMode: 'vertical-rl', textOrientation: 'mixed'}} instead of Tailwind utilities.
Suggestion: Use Tailwind's[writing-mode:vertical-rl]arbitrary property syntax or add a custom utility class. - 🟡
autogpt_platform/frontend/src/app/(platform)/copilot/components/ArtifactPanel/components/ArtifactDragHandle.tsx:76Drag handle has role="separator" and aria-label but no keyboard support (onKeyDown for arrow keys). Users who rely on keyboard cannot resize the panel, violating WCAG 2.1.1.
Suggestion: Add onKeyDown handler to support ArrowLeft/ArrowRight for keyboard-driven resizing, and add aria-valuemin/aria-valuemax/aria-valuenow attributes. - 🟢
autogpt_platform/frontend/src/app/(platform)/copilot/components/ArtifactPanel/components/ArtifactMinimizedStrip.tsx:22Expand button has title="Expand panel" but no aria-label. Screen readers may not reliably announce the button purpose from title alone.
Suggestion: Add aria-label="Expand artifact panel" to the button element. - 🟢
autogpt_platform/frontend/src/app/(platform)/copilot/components/ArtifactPanel/components/ArtifactPanelHeader.tsx:51HeaderButton lacks focus-visible ring styling, making it hard for keyboard users to identify the focused button. The minimized strip button correctly includes focus-visible:ring-2, creating inconsistency.
Suggestion: Add 'focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-violet-400' to the HeaderButton className. - 🟢
autogpt_platform/frontend/src/app/(platform)/copilot/components/ArtifactPanel/components/SourceToggle.tsx:12Toggle container div lacks role="group" and aria-label to semantically group the Preview/Source buttons for screen readers.
Suggestion: Add role="group" aria-label="View mode" to the container div. - 🟢
autogpt_platform/frontend/src/app/(platform)/copilot/components/ArtifactCard/ArtifactCard.tsx:42Non-openable artifact cards trigger immediate download on click without any confirmation, which could surprise users who expected an info tooltip or preview attempt.
Suggestion: Consider showing a small tooltip or visual indicator that clicking will download, or add a brief confirmation for larger files. - 🟢
autogpt_platform/frontend/src/app/(platform)/copilot/components/ChatContainer/useAutoOpenArtifacts.ts:87Auto-open fires unconditionally when a new artifact is detected, which can disrupt users who are actively reading or typing in the chat. There's no way for users to disable this behavior.
Suggestion: Consider adding a user preference or a one-time dismiss that suppresses auto-open for the rest of the session. - 🟡
autogpt_platform/frontend/src/app/(platform)/copilot/components/ChatContainer/useAutoOpenArtifacts.test.ts:1@0ubbe requested hook tests be restructured as component-level integration tests (render , assert behavior) rather than isolated hook unit tests. No response was given.
Suggestion: Acknowledge @0ubbe's feedback — either agree to refactor in a follow-up or explain the rationale for keeping hook-level tests. - 🟡
autogpt_platform/frontend/src/app/(platform)/copilot/store.test.ts:3@0ubbe noted store unit tests are too tightly coupled to implementation details. No response was given.
Suggestion: Acknowledge @0ubbe's comment and either plan to restructure or explain the testing approach. - 🟡
autogpt_platform/frontend/src/app/(platform)/copilot/components/ArtifactPanel/components/ArtifactContent.tsx:115PDF iframe has no sandbox attribute. @majdyz added sandbox='' in 48354ac but ntindle removed it claiming it breaks PDF viewers. The disagreement is unresolved — both reviewers made valid points.
Suggestion: Test across browsers (Chrome, Firefox, Safari) to determine if sandbox='' actually breaks PDF rendering, then document the decision. - 🟢
autogpt_platform/backend/backend/api/features/workspace/routes.py:344session_id query parameter on list_workspace_files accepts arbitrary strings with no format validation. Multiple bot reviewers flagged this repeatedly but it was never addressed.
Suggestion: Add UUID format validation or a regex constraint to session_id to prevent unexpected path constructions in WorkspaceManager. - 🟢
autogpt_platform/frontend/src/app/(platform)/copilot/components/ArtifactPanel/useArtifactPanel.ts:128@majdyz flagged that handleCopy swallows all errors with .catch(() => {}). Author acknowledged but didn't fix — user gets no feedback when copy fails.
Suggestion: Add a toast notification on copy failure for better UX.
Quality Checks
- ❌ lint: cd autogpt_platform/frontend && pnpm lint:
cd autogpt_platform/frontend && pnpm lint(1s) - ✅ lint: cd autogpt_platform/backend && poetry run lint:
cd autogpt_platform/backend && poetry run lint(69s) - ❌ typecheck: cd autogpt_platform/frontend && pnpm types:
cd autogpt_platform/frontend && pnpm types(0s) - ❌ test: cd autogpt_platform/backend && poetry run test:
cd autogpt_platform/backend && poetry run test(0s) - ❌ test: cd autogpt_platform/frontend && pnpm test:unit:
cd autogpt_platform/frontend && pnpm test:unit(0s) - ❌ build: cd autogpt_platform/frontend && pnpm build:
cd autogpt_platform/frontend && pnpm build(0s)
There was a problem hiding this comment.
📋 Automated Review — PR #12629
PR #12629 — feat(platform): add copilot artifact preview panel
Author: ntindle | Files: 45
🎯 Verdict: REQUEST_CHANGES
PR Description Quality
✅ Has Why + What + How — PR describes the artifact preview panel feature, its renderers, classification pipeline, and security model. Video demo provided.
What This PR Does
Adds a dedicated artifact preview panel to the copilot chat interface. When the AI assistant produces files (HTML, CSV, PDF, code, React components, images), they appear in a resizable side panel with type-specific renderers, copy/download actions, back/forward history navigation, and auto-open on new artifacts. The feature is gated behind Flag.ARTIFACTS. Backend additions include workspace file listing with pagination and metadata tracking for agent-created files.
Specialist Findings
🛡️ Security allow-scripts without allow-same-origin, SRI-pinned React CDN, proper escapeHtml, and </script> breakout prevention are all solid. However:
- 🟠 PDF iframe has no sandbox (
ArtifactContent.tsx:98) — Uses<iframe src={pdfUrl}>with zero sandbox attributes. While blob URLs have a null origin, an un-sandboxed iframe from a crafted PDF could attempt top-frame navigation. (Flagged by: security, architect — 2) - 🟡 HTML iframes can make outbound network requests (
ArtifactContent.tsx:119) —sandbox="allow-scripts"allowsfetch()/XHR to any host. Documented as a conscious tradeoff iniframe-sandbox-csp.ts. A CSPconnect-srcwhitelist would add defense-in-depth. - 🟡
stylesMarkupinjected raw into srcdoc (reactArtifactPreview.ts:63) — Safe today (hardcoded caller), but the function signature doesn't enforce this constraint. A JSDoc warning would prevent future misuse.
🏗️ Architecture ComponentName.tsx + useComponentName.ts + helpers.ts), proper feature flag gating, and good backend route conventions. Issues:
- 🟠 Inverted dependency (
store.ts:3) — Zustand store importsclearContentCachefrom a deeply nested component hook, inverting the dependency direction. Should be lifted to a shared service. - 🟠 Inconsistent renderer dispatch (
ArtifactContent.tsx:152) —codeRendereris imported directly while CSV/JSON/others go throughglobalRegistry.getRenderer(). Creates implicit coupling and divergence risk. - 🟡 Backend
list_fileslacks ordering guarantee (routes.py:370) — Thelimit+1pagination pattern assumes stable ordering, but noORDER BYis enforced in the API contract.
⚡ Performance ✅ — Content caching, blob URL cleanup, debounced persistence, and limit+1 pagination are all well-implemented. Minor optimization opportunities:
- 🟡 TypeScript compiler loaded without result caching (
transpileReactArtifact.ts:14) — ~5MB compiler runs on every React preview with no transpiled output cache. Preview↔Source toggling re-transpiles the same source. - 🟡 CSV renderer renders all rows to DOM (
CSVRenderer.tsx:113) —contentVisibility: "auto"helps paint but not React reconciliation for large CSVs (thousands of rows). - 🟡 Content cache unbounded by byte size (
useArtifactContent.ts:9) — 12 entries × up to 10MB each = ~120MB worst case in module-level memory. (Flagged by: performance, architect — 2)
🧪 Testing
- 🟠
useArtifactPanelhas zero tests (useArtifactPanel.ts:13) — The coordination hub containing Escape key handling, copy-from-cache fallback, viewport resize throttling,canCopyderivation, and effective width clamping is completely untested. (Flagged by: testing, discussion — 2) - 🟠
ArtifactRendererrouting untested (ArtifactContent.tsx:76) — 9 conditional rendering branches (image, PDF, HTML, React, code, JSON, CSV, registry fallback, plain text) with no test coverage. - 🟠 CSVRenderer tests are smoke-only (
CSVRenderer.test.ts:48) — Tests assertnot.toThrow()but never verify parsed output values for BOM stripping, quoted newlines, or escaped quotes. - 🟠 Backend soft-delete failure path untested (
routes.py:279) — Thetry/exceptaroundsoft_delete_workspace_filehas no test covering the exception path.
📖 Quality ✅ — Well-structured with clear naming, proper Tailwind usage, good docstrings on security decisions. Minor DRY issues:
- 🔵 Magic values duplicated (
ArtifactDragHandle.tsx:10,store.ts:65,useArtifactPanel.ts:127) —minWidth=320andmaxWidthPercent=85appear in three places. - 🔵 Tailwind injection duplicated (
ArtifactContent.tsx:119,HTMLRenderer.tsx:14) — Same CDN script +wrapWithHeadInjectionpattern in two locations. - 🔵
MAX_HISTORY = 25buried in action body (store.ts:205) — Should be module-level.
📦 Product
- 🟠 Image
<img>has noonErrorhandler (ArtifactContent.tsx:84) — A broken image URL shows the browser's broken-image icon with no retry. Every other content type has error handling. - 🟡 Auto-open disrupts active viewing (
useAutoOpenArtifacts.ts:1) — New artifacts hijack the panel from whatever the user is reading. No "user is interacting" suppression. - 🟡 PDF iframe has no browser fallback (
ArtifactContent.tsx:98) — Some mobile browsers can't render inline PDFs, leaving a blank panel.
📬 Discussion
- 🟠 Two cursor[bot] findings unacknowledged —
closeArtifactPanelnullifies artifact before exit animation (ArtifactPanel.tsx:42), andcanCopychecksclassification.label(display string) instead ofclassification.type(stable enum) (useArtifactPanel.ts:112). - 🟠 First-session auto-open bug acknowledged but untracked (
useAutoOpenArtifacts.ts:25) — Author agreed the first live artifact in a new session is skipped, but no GitHub issue was created. - 🟡 Contradictory signals on PDF sandbox — majdyz-bot said "Fixed in 48354ac" but ntindle replied "breaks browser PDF viewer, not fixing." Current code state should be documented.
🔎 QA session_id normalization all work correctly. Frontend: all 88 unit tests pass. Browser testing limited — copilot chat service was not running so artifact generation through the chat flow couldn't be triggered. The ARTIFACTS feature flag is correctly force-enabled via env var.
🟠 Should Fix
-
useArtifactPanelneeds test coverage (useArtifactPanel.ts:13) — Zero tests for the feature's coordination hub: Escape key handling, copy-from-cache, width clamping, viewport resize,canCopyderivation. This is the most impactful coverage gap. (Flagged by: testing, discussion — 2) -
ArtifactRendererrouting needs tests (ArtifactContent.tsx:76) — 9 rendering branches with type-specific metadata construction, none tested. Extract routing logic into a testable helper or add integration tests per type. (Flagged by: testing — 1) -
CSVRenderer tests need data assertions (
CSVRenderer.test.ts:48) — BOM handling and quoted-newline parsing are highlighted as hardening fixes but tests only assertnot.toThrow(). Add at least one assertion per edge case verifying the actual rendered cell content. (Flagged by: testing — 1) -
Image
<img>needsonErrorhandler (ArtifactContent.tsx:84) — Every other content type has graceful error handling; images show a broken icon with no retry. AddonErrortriggering the same error UI. (Flagged by: product — 1) -
canCopyshould checkclassification.type, not.label(useArtifactPanel.ts:112) — Checking the display string is fragile; a label change silently breaks the copy guard. Use the stabletypeenum. (Flagged by: discussion — 1) -
Respond to animation regression concern (
ArtifactPanel.tsx:42) —closeArtifactPanelnullifiesactiveArtifactimmediately, which may preventAnimatePresenceexit animation from rendering. Acknowledge or fix. (Flagged by: discussion — 1) -
Backend soft-delete failure path needs a test (
routes.py:279) — Thetry/exceptaroundsoft_delete_workspace_filelogs a warning but still raises 413. No test verifies this doesn't regress to 500. (Flagged by: testing — 1)
🟡 Nice to Have
- Lift
clearContentCacheto shared service (store.ts:3) — Fixes the inverted dependency where the store imports from a deeply nested component hook. (architect) - Route all renderers through
globalRegistry(ArtifactContent.tsx:152) — Removes the special-case direct import ofcodeRenderer. (architect) - Add byte-budget to content cache (
useArtifactContent.ts:9) — Cap total cache size at ~20-50MB, not just 12 entries. (performance, architect) - Cache transpiled React output by source hash (
transpileReactArtifact.ts:14) — Makes Preview↔Source toggling instant for repeat views. (performance) - Add
ORDER BYguarantee tolist_files(routes.py:370) — Ensures stable pagination. (architect) - Create tracking issue for first-session auto-open bug (
useAutoOpenArtifacts.ts:25) — Acknowledged by author but no issue exists. (discussion) - Suppress auto-open when user is actively viewing (
useAutoOpenArtifacts.ts:1) — Prevents hijacking the panel during streaming. (product) - PDF iframe fallback content (
ArtifactContent.tsx:98) — Show download link for browsers that can't render inline PDFs. (product, security) - Document PDF sandbox exception in
iframe-sandbox-csp.ts— Clarify the intentional omission alongside the existing CSP rationale. (architect, security)
🔵 Nits
- Magic values duplicated (
ArtifactDragHandle.tsx:10,store.ts:65) — ExportMIN_PANEL_WIDTHandMAX_PANEL_WIDTH_PERCENTas shared constants. MAX_HISTORYburied in action body (store.ts:205) — Move to module scope.collectPreviewStylescould be a const (reactArtifactPreview.ts:44) — Returns a static string with no parameters.- Tailwind injection duplicated (
ArtifactContent.tsx:119,HTMLRenderer.tsx:14) — Extract shared helper. - IIFE for
lastAssistantIdx(useAutoOpenArtifacts.ts:46) — Replace withmessages.findLastIndex(m => m.role === 'assistant'). - Download test boilerplate (
downloadArtifact.test.ts:22) — Extract mock setup intobeforeEach. - Fragile error dispatch via string prefix (
routes.py:279) — Consider typed exceptions instead ofmessage.startswith("File too large").
Human Review Needed
YES — 45-file feature addition with security-sensitive iframe sandboxing, new Function() eval of AI-generated code, and multiple unacknowledged reviewer comments. Frontend patch coverage at 38.72% is well below the 80% target. Human review should focus on the iframe security model decisions and the unresolved animation/canCopy feedback.
Risk Assessment
Merge risk: MEDIUM | Rollback: EASY (feature-flagged behind Flag.ARTIFACTS)
CI Status
❌ 2/6 quality checks passed locally. Frontend lint, typecheck, and build failed (likely dependency/generation issues in the test environment). Backend lint passed. Backend and frontend test suites could not be validated via CI commands but unit tests pass when run directly (88/88 ✅).
| // (Chromium bug #413851). The blob URL has a null origin so it can't | ||
| // access the parent page regardless. | ||
| return ( | ||
| <iframe src={pdfUrl} className="h-full w-full" title={artifact.title} /> |
There was a problem hiding this comment.
🤖 🟡 medium (security/Unsandboxed iframe)
PDF preview iframe has no sandbox attribute. While the blob: URL has a null origin, an un-sandboxed iframe from a crafted PDF could attempt top-frame navigation or trigger PDF JavaScript exploits.
Suggestion: Consider adding sandbox="" (empty sandbox — blocks all permissions including scripts) since the built-in PDF viewer doesn't need JS. If that breaks rendering, document why in a comment alongside the existing Chromium bug reference.
| const wrapped = wrapWithHeadInjection(content, tailwindScript); | ||
| return ( | ||
| <iframe | ||
| sandbox="allow-scripts" |
There was a problem hiding this comment.
🤖 🟡 medium (security/Outbound network from sandbox)
HTML artifact iframes with sandbox="allow-scripts" can make arbitrary outbound fetch/XHR requests. AI-generated or malicious HTML could exfiltrate artifact content or probe internal hosts from the user's browser IP.
Suggestion: This is a documented conscious tradeoff (iframe-sandbox-csp.ts). If you want defense-in-depth, consider a CSP with a restrictive connect-src whitelist for known CDN domains only, while allowing script-src 'unsafe-inline' for the embedded content.
| <meta charset="utf-8" /> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1" /> | ||
| <title>${safeTitle}</title> | ||
| ${stylesMarkup} |
There was a problem hiding this comment.
🤖 🟡 medium (security/Unsafe raw HTML injection)
The stylesMarkup parameter is injected raw into the HTML template. Currently safe (hardcoded caller), but the function signature doesn't enforce this — a future caller passing user-controlled content would create an XSS vector.
Suggestion: Add a JSDoc warning on the stylesMarkup parameter that it must be a trusted/hardcoded string, or validate it doesn't contain script tags.
| except ValueError as e: | ||
| raise fastapi.HTTPException(status_code=409, detail=str(e)) from e | ||
| # write_file raises ValueError for both path-conflict and size-limit | ||
| # cases; map each to its correct HTTP status. |
There was a problem hiding this comment.
🤖 🟢 low (security/Fragile error dispatch)
HTTP 413 vs 409 status code is dispatched by checking message.startswith("File too large"). If write_file changes its error message wording, the mapping silently regresses to 409 for size-limit errors.
Suggestion: Use a custom exception class (e.g., FileTooLargeError(ValueError)) instead of string prefix matching for more robust dispatch.
| ) | ||
|
|
||
|
|
||
| @router.get( |
There was a problem hiding this comment.
🤖 🟢 low (security/Pagination enumeration)
The new GET /files endpoint allows limit up to 1000. Repeated calls could enumerate large workspaces. Low risk since it's auth-gated.
Suggestion: Consider adding rate limiting on this endpoint if workspace enumeration becomes a concern.
| sessionId, | ||
| }: UseAutoOpenArtifactsOptions) { | ||
| const openArtifact = useCopilotUIStore((state) => state.openArtifact); | ||
| const messageFingerprintsRef = useRef<Map<string, string>>(new Map()); |
There was a problem hiding this comment.
🤖 🟡 medium (discussion/Deferred bug without tracking)
coderabbitai identified that the first live artifact in a brand-new session is skipped by auto-open logic. Author acknowledged but no tracking issue was created for follow-up.
Suggestion: Create a GitHub issue to track this bug so it doesn't get lost. It affects the core happy path of the feature.
| return ( | ||
| <iframe src={pdfUrl} className="h-full w-full" title={artifact.title} /> | ||
| ); | ||
| } |
There was a problem hiding this comment.
🤖 🟡 medium (discussion/Contradictory resolution)
majdyz-bot said 'Fixed in 48354ac: Added sandbox="" to PDF iframe', but ntindle later replied 'breaks browser PDF viewer, not fixing'. The final code state needs verification to determine which position prevailed.
Suggestion: Verify the current code and document the decision: if sandbox is omitted, add a code comment explaining why; if present, verify PDF rendering works cross-browser.
| <th | ||
| key={i} | ||
| className="px-3 py-2 text-left font-medium text-zinc-700" | ||
| > |
There was a problem hiding this comment.
🤖 🟢 low (discussion/Unacknowledged review feedback)
coderabbitai flagged that column sorting click handlers on elements are not keyboard-accessible. No response from author.
Suggestion: Use a inside the or add tabIndex={0} and onKeyDown handler for Enter/Space to ensure keyboard accessibility.
| // SSR fallback for viewport width before window is available. | ||
| const DEFAULT_VIEWPORT_WIDTH = 1280; | ||
|
|
||
| export function useArtifactPanel() { |
There was a problem hiding this comment.
🤖 🟢 low (discussion/Test coverage gap)
useArtifactPanel has zero test coverage despite orchestrating keyboard escape handling, copy-from-cache, effective width clamping, and maximize/viewport logic. Frontend patch coverage is 38.72% vs 80% target.
Suggestion: Add tests covering: Escape key closes panel, handleCopy uses cache, effectiveWidth clamping, maximized mode returns maxWidth.
| path=path, | ||
| mime_type=mime_type, | ||
| overwrite=overwrite, | ||
| metadata={"origin": "agent-created"}, |
There was a problem hiding this comment.
🤖 🟢 low (discussion/Test coverage gap)
The metadata={'origin': 'agent-created'} addition has no test verifying the metadata is passed through to write_file for agent-created files.
Suggestion: Add a test in the copilot tools test file that mocks WorkspaceManager.write_file and asserts metadata={'origin': 'agent-created'} is passed.
There was a problem hiding this comment.
📋 Automated Review — PR #12629
PR #12629 — feat(platform): add copilot artifact preview panel
Author: ntindle | Files: 45
🎯 Verdict: REQUEST_CHANGES
PR Description Quality
✅ Has Why + What + How — PR describes the new artifact preview panel, its renderers, auto-open behavior, and backend pagination additions clearly.
What This PR Does
Adds a copilot artifact preview panel that renders AI-generated files (HTML, CSV, PDF, React components, code, images) inline alongside the chat. The panel supports resize/minimize/maximize, keyboard shortcuts, copy/download, and auto-opens when the AI produces a new artifact. Backend changes add a paginated list_workspace_files endpoint and file metadata tracking. The entire feature ships behind a LaunchDarkly flag (Flag.ARTIFACTS, default off).
Specialist Findings
🛡️ Security allow-scripts only, no allow-same-origin), but two high-severity gaps exist.
- 🟠 PDF iframe has no sandbox attribute (
ArtifactContent.tsx:98) — While blob URLs have null origin, browser PDF plugins may execute embedded JS without restrictions. Addsandbox=""or document the threat model explicitly. - 🟠 Tailwind CDN loaded without SRI (
reactArtifactPreview.ts:97,ArtifactContent.tsx:115) — React/ReactDOM are pinned with SRI hashes, but the Tailwind JIT script is not. A CDN compromise would inject JS into every preview iframe. Author declined to fix citing JIT incompatibility with SRI; reviewer flagged as blocker. (Flagged by: security, discussion — 2 specialists) - 🟡
list_workspace_filesreturns internal file paths including session IDs (routes.py:382). Low exploitation risk but unnecessary exposure. - 🟡
stylesMarkupinterpolated unsanitized into srcDoc (reactArtifactPreview.ts:53). Safe today (static string) but no defense-in-depth.
🏗️ Architecture ✅ — Clean separation of concerns: Zustand store, classification pipeline, per-type renderers, and feature-flag gating all follow established codebase patterns.
- 🟡 Untyped
metadata: Optional[dict]onwrite_file(workspace.py:158) invites schema drift — aTypedDictwould be more defensive. - 🟡 Content cache keyed by artifact ID only (
useArtifactContent.ts:14) with no session boundary — stale content theoretically possible on session switch. - 🟡
require()shim in React preview (reactArtifactPreview.ts:183) only supports react/react-dom — AI-generated components importing third-party libraries will fail at runtime. Common failure mode for dashboards.
⚡ Performance ✅ — Adequate for interactive copilot UI. Good patterns: LRU caching, cancellation, debounced persistence.
- 🟡 Content cache allows 12 × 10MB = 120MB worst case (
useArtifactContent.ts:9). Add a total-bytes budget. (Flagged by: security, architect, performance — 3 specialists) - 🟡
extractWorkspaceArtifactscompiles two regexes per match inside a loop (helpers.ts:271). Pre-compile or useincludes(). - 🟡 Drag handle triggers Zustand store update on every pointermove (
ArtifactDragHandle.tsx:43). Use a local ref during drag, commit on pointerup.
🧪 Testing
- 🟠
useArtifactPanelhas zero tests (useArtifactPanel.ts:13) — Contains escape-key handling with dialog guard, viewport-responsive width clamping, copy-from-cache vs fetch fallback, andcanCopygating. All untested. (Flagged by: testing, prior review — carried forward) - 🟠 CSV renderer tests assert only
not.toThrow()(CSVRenderer.test.ts:49) — PR adds RFC 4180 quoted-newline parsing and BOM handling, but tests would pass even if the parser returned garbage. Need output-correctness assertions. - 🟡 Backend soft-delete failure path untested (
routes.py:282-286). Thetry/exceptlogs warning + raises 413 but no test covers whensoft_delete_workspace_fileitself throws. - 🟡 Empty-string
session_idnormalization untested (routes.py:360).
📖 Quality ✅ — Readability score: A. Clean naming, proper JSDoc, no any types, no legacy imports.
- 🔵 Magic width constants (
320,85%) duplicated acrossArtifactDragHandle.tsx:12,store.ts:65,useArtifactPanel.ts:127. Extract shared constants. - 🔵 Download-with-toast pattern duplicated in
ArtifactCard.tsx:39anduseArtifactPanel.ts:114. Extract shared helper. - 🔵
reactArtifactPreview.tsat 318 lines with a 260-line HTML template string. Consider extracting the runtime script.
📦 Product ✅ — Feature-complete implementation matching all stated requirements. Feature flag makes rollback trivial.
- 🟡 PDF iframe has no loading indicator (
ArtifactContent.tsx:97) — blank white rectangle while PDF viewer initializes. - 🟡 Drag handle has no keyboard resize support (
ArtifactDragHandle.tsx:76) —role="separator"without arrow key handling means pointer-only resize. - 🟡 Auto-open may interrupt user mid-typing (
useAutoOpenArtifacts.ts) — no debounce or input-focus gate.
📬 Discussion
- 🟠 @0ubbe's testing strategy comments completely unanswered — Two substantive comments about preferring component-level integration tests over isolated hook/store tests received zero response. This is the only human code reviewer.
- 🟠 Tailwind CDN SRI disagreement unresolved — Flagged as blocker by automated reviewer; author declined to fix. No formal resolution.
- 🟡
canCopyusesclassification.label(display string) instead ofclassification.type— flagged by Cursor bot, no response. (Flagged by: discussion, product — 2 specialists) - 🟡
useAutoOpenArtifactsnew-session first-artifact bug acknowledged but unresolved. Author said "will investigate."
🔎 QA
🟠 Should Fix
useArtifactPanelneeds tests (useArtifactPanel.ts:13) — Zero coverage on a hook with escape-key handling, width clamping, copy fallback logic, andcanCopygating. Add tests for: (1) Escape closes panel but not when dialog is open, (2)canCopyreturns false for image/pdf/download-only, (3)handleCopyuses cache when available vs fetches when not, (4)effectiveWidthclamped in maximize mode. (Flagged by: testing, prior review — 2)- CSV renderer tests need output-correctness assertions (
CSVRenderer.test.ts:49) — Current tests only assertnot.toThrow(). Add assertions that verify parsed output: correct row count for embedded newlines, BOM stripped from first header cell, quoted fields properly unescaped. (Flagged by: testing — 1) - Respond to @0ubbe's testing strategy feedback — Two substantive comments from the only human code reviewer have zero acknowledgment. Even a brief "noted for follow-up" or a rationale for the current approach would resolve this. (Flagged by: discussion — 1)
- Resolve Tailwind CDN SRI disagreement (
reactArtifactPreview.ts:97) — If SRI truly isn't feasible for the JIT CDN, pin to a specific version URL and add a code comment documenting the security rationale and sandbox mitigation. This closes the loop with reviewers without requiring SRI. (Flagged by: security, discussion — 2)
🟡 Nice to Have
- Add
sandbox=""to PDF iframe (ArtifactContent.tsx:98) — Low-probability risk given blob URLs, but inconsistent with the otherwise thorough sandbox model. Test in Chrome/Edge before shipping. (security) - Type the
metadataparameter (workspace.py:158) — ReplaceOptional[dict]with aTypedDictto prevent schema drift as more callers are added. (architect) - Add total-bytes budget to content cache (
useArtifactContent.ts:9) — 12 entries × 10MB = 120MB worst case. A 20MB total cap with oldest-entry eviction would bound memory. (security, architect, performance) - Cache PDF blob URLs (
useArtifactContent.ts:79) — Re-opening the same PDF triggers a full re-download. Cache blob URL in the LRU, revoke on eviction. (performance) - Add keyboard resize to drag handle (
ArtifactDragHandle.tsx:76) — Arrow key support on therole="separator"element for accessibility. (product) - Pre-compile regexes in
extractWorkspaceArtifacts(helpers.ts:271) — MoveimagePattern/linkPatternconstruction outside the loop. (performance)
🔵 Nits
- Extract shared width constants (
ArtifactDragHandle.tsx:12,store.ts:65,useArtifactPanel.ts:127) —320and85%/0.85are duplicated across three files. (quality) - Extract download-with-toast helper (
ArtifactCard.tsx:39,useArtifactPanel.ts:114) — Near-identicaldownloadArtifact(...).catch(toast)pattern in two files. (quality) - Named interface for ArtifactRenderer props (
ArtifactContent.tsx:72) — 5-property inline type on a 120-line function. (quality)
Human Review Needed
YES — 45-file feature PR with security-sensitive iframe sandboxing, new backend endpoints, and unacknowledged human reviewer feedback. Feature flag reduces risk, but the sandbox security model and testing gaps warrant human sign-off.
Risk Assessment
Merge risk: MEDIUM | Rollback: EASY (behind Flag.ARTIFACTS, default off)
CI Status
❌ Local quality checks show failures in frontend lint, typecheck, build, and both test suites (likely environment/dependency issues). Remote CI on GitHub shows ✅ 46/46 checks passing. Merge status is UNKNOWN — may need rebase.
| // (Chromium bug #413851). The blob URL has a null origin so it can't | ||
| // access the parent page regardless. | ||
| return ( | ||
| <iframe src={pdfUrl} className="h-full w-full" title={artifact.title} /> |
There was a problem hiding this comment.
🤖 🟠 high (security/Unsandboxed iframe)
PDF iframe has no sandbox attribute. While blob URLs have a null origin, browser PDF plugins may execute embedded JS without any sandbox restrictions.
Suggestion: Add sandbox="" (empty — no permissions) to the PDF iframe. If that breaks rendering, consider sandbox="allow-same-origin" (safe since blob has null origin) or at minimum document the threat model for PDFs with embedded JS.
| white-space: pre-wrap; | ||
| } | ||
| </style> | ||
| <script src="${TAILWIND_CDN_URL}"></script> |
There was a problem hiding this comment.
🤖 🟠 high (security/Missing SRI on CDN script)
Tailwind CDN script is loaded without an integrity attribute, unlike the React/ReactDOM scripts which use SRI. A CDN compromise would inject arbitrary JS into every preview iframe.
Suggestion: Pin the Tailwind CDN with an SRI hash, or self-host the Tailwind standalone build. The comment says 'SRI is not possible because the JIT runtime is generated on demand' — consider using the pre-built Tailwind CSS file instead of the JIT script if SRI is needed.
| id=f.id, | ||
| name=f.name, | ||
| path=f.path, | ||
| mime_type=f.mime_type, |
There was a problem hiding this comment.
🤖 🟡 medium (security/Internal path exposure)
list_workspace_files returns f.path which contains internal directory structure including session IDs (e.g. /sessions/{session_id}/filename). This leaks session identifiers via the API response.
Suggestion: Either strip the session prefix from the returned path or omit the path field entirely — the frontend only needs id, name, and mime_type for artifact display.
| * artifact preview iframes. CSP was added and removed multiple times during | ||
| * review — here's why it stays out: | ||
| * | ||
| * 1. `connect-src 'none'` breaks any AI-generated HTML that uses fetch(), |
There was a problem hiding this comment.
🤖 🟡 medium (security/No network restriction in sandbox)
HTML artifact iframes can make arbitrary network requests (fetch/XHR/WebSocket) to any external host. AI-generated HTML could exfiltrate artifact content or use the user's browser as a request proxy.
Suggestion: Consider adding a CSP connect-src directive that allows only specific domains needed for dashboards (e.g. common chart data APIs) rather than leaving it fully open, or accept the risk with clear documentation.
| compiledCode: string, | ||
| title: string, | ||
| stylesMarkup: string, | ||
| ) { |
There was a problem hiding this comment.
🤖 🟡 medium (security/Unsanitized template interpolation)
stylesMarkup parameter is interpolated directly into the srcDoc HTML template without escaping. Currently safe because collectPreviewStyles() returns a static string, but any future dynamic content would be an injection vector.
Suggestion: Add a comment warning against dynamic content in stylesMarkup, or validate/escape it before interpolation to be defense-in-depth.
| @@ -0,0 +1,140 @@ | |||
| import { act, renderHook } from "@testing-library/react"; | |||
There was a problem hiding this comment.
🤖 🟡 medium (discussion/Unaddressed reviewer feedback)
@0ubbe commented that hooks should not be tested separately from components — suggested component-level integration tests in tests/main.test.tsx instead. No response from the author.
Suggestion: Acknowledge @0ubbe's feedback and either migrate to component-level integration tests or explain why isolated hook tests are appropriate here.
| @@ -0,0 +1,141 @@ | |||
| import { beforeEach, describe, expect, it } from "vitest"; | |||
There was a problem hiding this comment.
🤖 🟡 medium (discussion/Unaddressed reviewer feedback)
@0ubbe flagged that store unit tests are too tightly coupled to implementation details and the team norm is to test components directly. No response from the author.
Suggestion: Respond to @0ubbe's comment — either acknowledge the concern for a follow-up or explain the rationale for keeping these tests.
| * | ||
| * React is loaded from unpkg with pinned version and SRI integrity hashes. | ||
| */ | ||
|
|
There was a problem hiding this comment.
🤖 🟡 medium (discussion/Unresolved disagreement)
Tailwind CDN (cdn.tailwindcss.com) is loaded without version pin or SRI hash — flagged as a Blocker by automated reviewer. Author declined to fix citing JIT CDN nature and sandbox isolation. Disagreement is unresolved.
Suggestion: Consider pinning to a specific Tailwind Play CDN version URL (e.g., cdn.tailwindcss.com/3.4.1) even without SRI, or document the security rationale in a code comment.
| @@ -0,0 +1,91 @@ | |||
| "use client"; | |||
There was a problem hiding this comment.
🤖 🟢 low (discussion/Known bug deferred)
Author acknowledged a bug where the first artifact in a brand-new session is skipped (hydration vs live message distinction). Said 'Will investigate — tracking for follow-up' but no fix committed.
Suggestion: File a tracking issue for this known bug before merging so it doesn't get lost.
| @@ -0,0 +1,148 @@ | |||
| "use client"; | |||
There was a problem hiding this comment.
🤖 🟢 low (discussion/Fragile string check)
Cursor bot flagged that canCopy uses classification.label (display string) instead of classification.type for gating. No response from the author.
Suggestion: Switch the canCopy check to use classification.type instead of classification.label for consistency and robustness.
There was a problem hiding this comment.
📋 Automated Review — PR #12629
PR #12629 — feat(platform): add copilot artifact preview panel
Author: ntindle | Files: 45
🎯 Verdict: REQUEST_CHANGES
PR Description Quality
✅ Has Why + What + How — Clear description covering the artifact preview panel feature, backend workspace API improvements, and security model decisions.
What This PR Does
Adds a dedicated artifact preview panel to the copilot interface that can render HTML, CSV, PDF, code, and React artifacts in sandboxed iframes. Includes backend workspace API enhancements (pagination, metadata origin tracking, proper error codes), content caching with LRU eviction, resize/minimize/maximize controls, auto-open behavior with fingerprint-based dedup, and download/copy actions. The entire feature is gated behind a Flag.ARTIFACTS feature flag for safe rollout.
Specialist Findings
🛡️ Security sandbox="allow-scripts" without allow-same-origin), auth on all backend endpoints, good escaping for </script> breakout and title injection. However, Tailwind CDN is loaded without SRI in every artifact iframe, and no CSP restricts outbound requests from sandboxed content.
- 🟠 Tailwind CDN without SRI (
iframe-sandbox-csp.ts:33) — Unlike React/ReactDOM which have SRI pinning, Tailwind's JIT runtime is loaded without integrity verification. A CDN compromise injects arbitrary JS into every preview. (Flagged by: security, architect — 2) - 🟠 No CSP on artifact iframes (
ArtifactContent.tsx:119) — Sandboxed iframes allow unrestrictedfetch/XHR, enabling browser-side SSRF, local network port scanning, or content exfiltration from AI-generated HTML. Documented as accepted risk but should have explicit CSP or threat model note. (Flagged by: security) - 🟡 Unsandboxed PDF iframe (
ArtifactContent.tsx:98) — PDF iframe omitssandboxdue to Chromium bug #413851. Blob URL has null origin today but relies on browser semantics not changing. (Flagged by: security, architect — 2)
🏗️ Architecture ✅ — Clean Component.tsx + useComponent.ts + helpers.ts pattern throughout. Zustand store properly co-located. Feature flag gating, dynamic imports, and Phosphor-only icons all follow repo conventions.
- 🟠 Fragile error dispatch via string matching (
routes.py:278) — HTTP 413 vs 409 determined bystartswith("File too large")on ValueError message. Ifwrite_filechanges wording, wrong status code is returned silently. Should use typed exception subclasses. (Flagged by: architect) - 🟡 Module-level content cache lifecycle (
useArtifactContent.ts:14) —contentCacheMap survives session switches (only cleared on logout). Theoretical stale-content risk with file ID reuse across sessions. (Flagged by: architect, performance — 2)
⚡ Performance ✅ — Good caching (LRU with 12-entry cap), debounced localStorage persistence, paginated backend listing with limit+1 trick. No critical perf issues.
- 🟡 TypeScript transpilation on main thread (
transpileReactArtifact.ts:14) — Full TS compiler (~3MB) loaded and executed on main thread. Will block UI for complex React artifacts. Moving to a Web Worker would eliminate jank. (Flagged by: performance) - 🟡 Zustand over-subscription during drag (
useArtifactPanel.ts:14) —useCopilotUIStore((s) => s.artifactPanel)returns the entire panel state object, causing ~60Hz re-renders during resize drag. Individual selectors oruseShallowwould reduce unnecessary re-renders. (Flagged by: performance)
🧪 Testing session_id normalization lacks regression tests, and useArtifactPanel hook has zero coverage for its most important logic paths.
- 🟠 CSVRenderer tests are smoke-only (
CSVRenderer.test.ts:49) — Allrender()tests usenot.toThrow()with no assertions on parsed values. A broken parser returning empty rows passes all tests. (Flagged by: testing) - 🟠 Missing
session_idnormalization regression test (routes_test.py) — Empty-stringsession_idnormalization toNone(a bug fix in this PR) has no test. Regression would silently break session scoping. (Flagged by: testing) - 🟠
useArtifactPanelhas zero test coverage (useArtifactPanel.ts:89) — Copy-from-cache-with-fetch-fallback, Escape key handler,canCopyderivation, andeffectiveWidthclamping are all untested. This was flagged in the prior review and remains unaddressed. (Flagged by: testing, prior review — escalated)
📖 Quality ✅ — Readability is strong (A-). Good naming, consistent formatting, proper use of function declarations. Security model well-documented in iframe-sandbox-csp.ts.
- 🟡 Magic value duplication (
ArtifactDragHandle.tsx:17,store.ts:64,useArtifactPanel.ts:127) —maxWidthPercent=85hardcoded in three places that can drift independently. Should be a shared constant. (Flagged by: quality) - 🔵
reactArtifactPreview.tsat 318 lines — Exceeds ~200 line guideline due to embedded JS runtime. Extracting the runtime would improve maintainability. (Flagged by: quality, architect — 2)
📦 Product ✅ — Feature complete per PR description. Good UX fundamentals: skeleton loaders, error retry, 10MB+ file gating, scroll position persistence, proper feature flag gating.
- 🟠 Drag handle not keyboard-operable (
ArtifactDragHandle.tsx:76) — Hasrole="separator"andaria-labelbut noArrowLeft/ArrowRightkey handler. WCAG requires keyboard-operable separators. (Flagged by: product) - 🟡 React preview error is a dead end (
ArtifactReactPreview.tsx:47) — No retry button on transpile/render errors, unlike the main content loader. Users must close and reopen. (Flagged by: product) - 🟡 Image
<img>has noonErrorfallback (ArtifactContent.tsx:84) — Broken image URLs show the browser's broken-image icon with no user feedback. (Flagged by: product)
📬 Discussion ✅ — All CI checks pass (48/48). One human approval (@0ubbe) is current and covers all commits. Author addressed all substantive reviewer concerns with fix commits. Two testing-approach comments from @0ubbe are unacknowledged but non-blocking (approver still approved). Frontend patch coverage at 38.72% (target 80%).
🔎 QA ✅ — All backend API endpoints verified (list, upload, download, delete, storage, pagination, path traversal, duplicate upload). Frontend loads without regressions. 110 tests pass (22 backend + 88 frontend). Artifact panel UI could not be exercised end-to-end (requires LLM keys to generate artifacts), but comprehensive unit tests validate all logic paths.
🟠 Should Fix
- CSVRenderer tests need actual value assertions (
CSVRenderer.test.ts:49) — Add at least one test that asserts parsed headers and cell values match expected output. Current smoke-only tests would pass even with a completely broken parser. (Flagged by: testing) - Add
session_id=""normalization regression test (routes_test.py) — This PR fixes a bug where empty-string session IDs caused incorrect scoping. Without a regression test, it will silently break again. (Flagged by: testing) - Add
useArtifactPanelcopy-path tests (useArtifactPanel.ts:89) — The cache-hit and fetch-fallback copy paths, plus error handling, are the most user-facing untested logic in this PR. This was flagged in the prior review and remains unaddressed. (Flagged by: testing, prior review — escalated) - Replace string-matching error dispatch with typed exceptions (
routes.py:278) —startswith("File too large")is fragile. CreateFileTooLargeErrorandFileConflictErrorsubclasses. (Flagged by: architect) - Add keyboard resize to drag handle (
ArtifactDragHandle.tsx:76) — AddonKeyDownhandler forArrowLeft/ArrowRightto meet WCAG operable separator requirements. (Flagged by: product)
🟡 Nice to Have
- Self-host Tailwind CSS instead of CDN (
iframe-sandbox-csp.ts:33) — Eliminates supply-chain risk from unpinned CDN. Complex to implement due to JIT nature. (security, architect) - Add CSP
connect-srcto artifact iframes (ArtifactContent.tsx:119) — Would restrict outbound requests from sandboxed content. Requires scoping allowed domains. (security) - Move TS transpilation to Web Worker (
transpileReactArtifact.ts:14) — Prevents main-thread blocking on complex React artifacts. (performance) - Use individual Zustand selectors or
useShallow(useArtifactPanel.ts:14) — Reduces unnecessary re-renders during panel drag resize. (performance) - Extract shared
maxWidthPercentconstant (ArtifactDragHandle.tsx:17,store.ts:64,useArtifactPanel.ts:127) — Three files hardcode0.85/85independently. (quality) - React preview error retry button (
ArtifactReactPreview.tsx:47) — Dead-end error state should match main content loader's retry pattern. (product) - Image
onErrorfallback (ArtifactContent.tsx:84) — Show user-friendly message instead of browser broken-image icon. (product)
🔵 Nits
- Move
MAX_HISTORYto module level (store.ts:205) — Currently declared inline insideopenArtifactaction body. reactArtifactPreview.ts:23re-export — Acts as a mini barrel file; direct imports would follow the "no barrel files" convention.- Delay
URL.revokeObjectURL(downloadArtifact.ts:33) — Synchronous revocation aftera.click()may break downloads in Firefox. UsesetTimeout(..., 1000).
QA Screenshots
| Screenshot | Description |
|---|---|
 |
Copilot page renders with chat interface ✅ |
 |
Error gracefully displayed (expected — no LLM keys in test env) ✅ |
 |
Build page loads without regressions ✅ |
Human Review Needed
YES — 45 files changed across frontend and backend, security-sensitive iframe sandboxing model, new backend API endpoints with auth, and a complex client-side state management system. Human reviewer should validate the iframe security trade-offs (no CSP, Tailwind CDN trust) and confirm the session_id authorization boundary in WorkspaceManager.list_files.
Risk Assessment
Merge risk: MEDIUM | Rollback: EASY (feature-flagged behind Flag.ARTIFACTS)
CI Status
❌ 2/6 quality checks failed (frontend lint, frontend typecheck); 3 checks errored (backend test, frontend test, frontend build — likely infrastructure/env issues). Backend lint passes. Note: All 48 GitHub CI checks on the PR itself pass green — local quality check failures appear to be environment-specific.
Prior Review Delta
| Prior Finding | Status |
|---|---|
useArtifactPanel has zero tests |
❌ Still open — escalated to 🟠 Should Fix |
| CSVRenderer smoke-only tests | ❌ Still open — remains 🟠 Should Fix |
| Animation regression (unacknowledged) | ✅ Addressed — no longer flagged by any specialist |
Fragile canCopy check |
canCopy derivation exists but remains untested |
|
|
||
| // Pinned to a specific version to reduce exposure to unannounced upstream | ||
| // changes (SRI is not possible because the JIT runtime is generated on demand). | ||
| export const TAILWIND_CDN_URL = "https://cdn.tailwindcss.com/3.4.16"; |
There was a problem hiding this comment.
🤖 🟠 high (security/CDN trust without SRI)
Tailwind CDN script is loaded without Subresource Integrity (SRI) hash in both HTML and React artifact iframes. Unlike the React/ReactDOM bundles which have SRI pinning, a CDN compromise could inject arbitrary JS into every artifact preview.
Suggestion: Pin to a specific Tailwind CSS build served from your own CDN or a versioned URL with an SRI hash. If the JIT nature makes SRI impossible, consider bundling a static Tailwind CSS file instead of loading the runtime script.
| const wrapped = wrapWithHeadInjection(content, tailwindScript); | ||
| return ( | ||
| <iframe | ||
| sandbox="allow-scripts" |
There was a problem hiding this comment.
🤖 🟠 high (security/No CSP on artifact iframes)
HTML artifact iframes use sandbox='allow-scripts' but have no CSP, allowing unrestricted outbound fetch/XHR. AI-generated or user-crafted HTML can make requests to internal network services (browser-side SSRF), perform local network port scanning, or exfiltrate artifact content.
Suggestion: Consider adding a restrictive connect-src CSP that whitelists only known external domains, or document this as explicitly accepted risk with a threat model note.
| // (Chromium bug #413851). The blob URL has a null origin so it can't | ||
| // access the parent page regardless. | ||
| return ( | ||
| <iframe src={pdfUrl} className="h-full w-full" title={artifact.title} /> |
There was a problem hiding this comment.
🤖 🟡 medium (security/Unsandboxed PDF iframe)
PDF artifacts are rendered in an iframe without any sandbox attribute. While blob URLs have null origin, PDF viewers have their own JS engine and a history of sandbox escapes. This grants more capability than the HTML/React preview paths.
Suggestion: Document this as accepted risk due to the Chrome limitation. Consider adding sandbox='allow-scripts allow-same-origin' if future Chrome versions support it, or use PDF.js as a renderer instead.
| WorkspaceFileItem( | ||
| id=f.id, | ||
| name=f.name, | ||
| path=f.path, |
There was a problem hiding this comment.
🤖 🟡 medium (security/Session path exposure)
The list_workspace_files endpoint returns the full file path (e.g., /sessions/sess-123/file.txt), leaking session IDs to the frontend. If session IDs are sensitive, this could enable enumeration.
Suggestion: Consider returning only the filename or a relative path without the session prefix, or confirm that session IDs are not security-sensitive.
| <meta charset="utf-8" /> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1" /> | ||
| <title>${safeTitle}</title> | ||
| ${stylesMarkup} |
There was a problem hiding this comment.
🤖 🟡 medium (security/Unsanitized stylesMarkup injection)
The stylesMarkup parameter is interpolated directly into the srcdoc HTML template without escaping. Currently safe because collectPreviewStyles() returns a hardcoded string, but fragile if any future caller passes user-derived data.
Suggestion: Add a comment marking stylesMarkup as trusted-only, or sanitize it before interpolation.
|
|
||
| export function SourceToggle({ isSourceView, onToggle }: Props) { | ||
| return ( | ||
| <div className="flex items-center rounded-md border border-zinc-200 bg-zinc-50 p-0.5 text-xs font-medium"> |
There was a problem hiding this comment.
🤖 🟢 low (product/Accessibility)
The toggle button group container lacks role="group" and aria-label, so screen readers don't understand the two buttons are related.
Suggestion: Add role="group" aria-label="View mode" to the container div.
|
|
||
| return ( | ||
| <div className="flex h-full w-10 flex-col items-center border-l border-zinc-200 bg-white pt-3"> | ||
| <button |
There was a problem hiding this comment.
🤖 🟢 low (product/Accessibility)
Expand button uses title but not aria-label. Screen readers may not consistently announce the title attribute.
Suggestion: Add aria-label="Expand panel" to the button alongside the existing title.
| @@ -0,0 +1,140 @@ | |||
| import { act, renderHook } from "@testing-library/react"; | |||
There was a problem hiding this comment.
🤖 🟢 low (discussion/Unaddressed review feedback)
@0ubbe suggested testing hooks via component integration tests (rendering ) rather than isolated hook tests. No response from author.
Suggestion: Consider acknowledging 0ubbe's testing approach feedback — either adopt component-level integration tests or explain why hook-level testing is preferred here.
| @@ -0,0 +1,141 @@ | |||
| import { beforeEach, describe, expect, it } from "vitest"; | |||
There was a problem hiding this comment.
🤖 🟢 low (discussion/Unaddressed review feedback)
@0ubbe noted these store unit tests are too tightly coupled to implementation details and suggested testing via components instead. No response from author.
Suggestion: Acknowledge the feedback and consider whether component-level tests would provide better regression value with less maintenance burden.
| @@ -0,0 +1,125 @@ | |||
| "use client"; | |||
There was a problem hiding this comment.
🤖 🟢 low (discussion/Coverage gap)
Frontend patch coverage is 38.72% against an 80% target. Most new UI components (ArtifactPanel, ArtifactCard, renderers) lack test coverage.
Suggestion: Add component-level integration tests for the new ArtifactPanel and ArtifactCard components to improve coverage.
There was a problem hiding this comment.
📋 Automated Review — PR #12629
PR #12629 — feat(platform): add copilot artifact preview panel
Author: ntindle | Files: 45
🎯 Verdict: REQUEST_CHANGES
PR Description Quality
✅ Has Why + What + How — PR describes the artifact preview panel feature, its rendering pipeline, security model, and backend workspace file management changes.
What This PR Does
Adds a copilot artifact preview panel that automatically detects and renders file artifacts (HTML, CSV, PDF, code, React/TSX) generated during copilot conversations. The panel supports resize/minimize/maximize, history navigation, download/copy actions, and mobile Sheet overlay. Backend changes add a GET /workspace/files endpoint with pagination and file origin metadata tracking. The entire feature is gated behind a Flag.ARTIFACTS LaunchDarkly flag for safe incremental rollout.
Specialist Findings
🛡️ Security allow-scripts without allow-same-origin) is well-designed and documented. </script> breakout prevention, HTML title escaping, and filename sanitization are all correct and tested. Two high-severity items remain from the prior review:
- 🟠 Tailwind CDN without SRI (
iframe-sandbox-csp.ts:37) — Every artifact iframe loads Tailwind fromcdn.tailwindcss.comwithout Subresource Integrity. React/ReactDOM have SRI pinning but Tailwind does not; a CDN compromise would inject JS into all artifact previews. The JIT nature makes SRI infeasible, but self-hosting the script would eliminate the third-party trust dependency. (Flagged by: security, QA — 2) - 🟠 PDF iframe without sandbox (
ArtifactContent.tsx:102) — PDF preview renders in an unsandboxed iframe due to Chromium bug #413851. The blob URL provides an opaque origin which limits blast radius, but this should be documented inline with the same rigor as the HTML/React iframe security comments. (Flagged by: security, architect — 2) - 🟡 Outbound network from sandboxed iframes is a documented accepted risk (
reactArtifactPreview.ts:14). Malicious artifacts can make arbitrary fetch/XHR. - 🟡
ValueErrormessage strings returned to client in upload errors could leak internal path info (routes.py:280).
🏗️ Architecture ✅ — Clean separation of concerns following CONTRIBUTING.md patterns: ArtifactPanel.tsx + useArtifactPanel.ts + helpers.ts + local components/. Zustand store with granular selectors. Feature flag gating with next/dynamic SSR-disabled import.
- 🟠 Fragile string-based error mapping (
routes.py:278-283) — The 413 vs 409 HTTP status depends onmessage.startswith("File too large"). A backend message change silently breaks the status mapping. Should use typed exception subclasses. (Flagged by: architect, quality — 2) - 🟡 Untyped
metadata: Optional[dict]parameter (workspace.py:158) should become aTypedDictas usage grows.
⚡ Performance ✅ — Acceptable for a feature-flagged UI panel. Content caching with LRU eviction and fingerprint-based message scanning show performance was considered.
- 🟡 TypeScript
transpileModuleruns on main thread (transpileReactArtifact.ts:14) — will cause 100-500ms jank for large React artifacts. Web Worker would be ideal follow-up. - 🟡 CSV renders all rows without virtualization (
CSVRenderer.tsx:113) —contentVisibility: autohelps paint but doesn't prevent 10K+ DOM nodes. - 🟡
useArtifactPanelsubscribes to entireartifactPanelsub-object (useArtifactPanel.ts:14), causing re-renders at ~60Hz during drag resize.
🧪 Testing
- 🟠
useArtifactPanel.tshas zero test coverage — keyboard shortcuts (Escape), copy with cache-vs-fetch branching, download error handling, and viewport width clamping are all untested. (Flagged by: testing — 1) - 🟠
ArtifactContent.tsxrenderer dispatch untested — The routing logic (HTML→iframe, React→preview, code→codeRenderer, JSON→parse, CSV→csvRenderer, fallback→<pre>) has no integration tests. A broken dispatch silently shows raw text. (Flagged by: testing — 1) - 🟠 Empty-string
session_idnormalization untested (routes.py:218, 361) — Both upload and list endpoints normalize""toNone, but no test verifies this. A regression would bypass session scoping. (Flagged by: testing — 1) - 🟠 CSVRenderer smoke tests have weak assertions (
CSVRenderer.test.ts:55) — BOM-stripping test only assertsnot.toThrow()without verifying the BOM is actually removed from parsed output. - 🟡 Backend copilot tool
metadata={"origin": "agent-created"}(workspace_files.py:848) untested. - 🟡 Frontend patch coverage at 38.72% vs 80% target per CI report.
📖 Quality ✅ — Readability is strong. Naming conventions, Tailwind usage, Phosphor Icons, function declarations for components all follow AGENTS.md. Security decisions are documented with exemplary block comments (especially iframe-sandbox-csp.ts).
- 🟡
reactArtifactPreview.tsat 318 lines exceeds the 200-line guideline. - 🔵
ArtifactMinimizedStrip.tsx:36inlinestyleobject recreated every render — extract to module-level constant.
📦 Product ✅ — Complete feature with defensive coding (size gating, sandbox isolation, filename sanitization, feature flag). Error states, loading states, and mobile experience are all handled.
- 🟡 Auto-open may be disruptive during active streaming (
useAutoOpenArtifacts.ts:87). - 🟡 Drag handle lacks keyboard resize — ARIA
separatorrole should support arrow keys (ArtifactDragHandle.tsx:76). (Flagged by: product, discussion — 2) - 🟡 CSS
text-overflow: ellipsisdoesn't work withwriting-mode: vertical-rlin minimized strip (ArtifactMinimizedStrip.tsx:33).
📬 Discussion
- 🟠 No human approvals on record —
reviewDecision: APPROVEDappears to come from CI satisfaction, not explicit reviewer approval. 11 commits pushed after last reviewer feedback without re-review. - 🟡
canCopycheck uses fragileclassification.labelstring instead ofclassification.type(useArtifactPanel.ts:112). - 🟡 Author acknowledged auto-open may skip first artifact in new sessions — tracked for follow-up but unfixed.
🔎 QA </script> injection prevention, and feature flag gating. Backend routes_test.py provides good endpoint coverage.
- 🟡
downloadArtifact.ts:32callsURL.revokeObjectURLsynchronously aftera.click()— on slow machines the download may not have started. - 🟡 No React error boundary wrapping
ArtifactContent— a synchronous renderer throw propagates to parent.
🟠 Should Fix
- Add
useArtifactPaneltests (useArtifactPanel.ts) — This hook contains keyboard shortcuts, copy branching (cache vs fetch), download error handling, and viewport clamping — all regressionable logic with zero coverage. AddrenderHooktests for Escape-to-close, handleCopy cache path, handleCopy fetch fallback, and effectiveWidth constraint. (Flagged by: testing — 1) - Add
ArtifactContentrenderer dispatch tests (ArtifactContent.tsx:75) — The type-routing logic dispatches to 6 different renderers with no integration tests. A broken dispatch silently degrades all artifact previews. (Flagged by: testing — 1) - Test empty-string
session_idnormalization (routes.py:218, 361) — Both upload and list endpoints normalizesession_id = session_id or None. No test verifies?session_id=behaves the same as omitting it. A regression here bypasses session scoping. (Flagged by: testing — 1) - Strengthen CSVRenderer assertions (
CSVRenderer.test.ts:55) — BOM-stripping and CRLF tests only assertnot.toThrow(). Assert actual parsed cell values to catch silent data corruption. (Flagged by: testing — 1) - Replace string-based error mapping with typed exceptions (
routes.py:278-283) —message.startswith("File too large")for 413 vs 409 is fragile. IntroduceFileTooLargeError(ValueError)andFileConflictError(ValueError)and useisinstance(). (Flagged by: architect, quality — 2) - Document or sandbox PDF iframe (
ArtifactContent.tsx:102) — Add inline security documentation explaining the opaque origin isolation model, matching the rigor of the HTML/React iframe comments. (Flagged by: security, architect — 2)
🟡 Nice to Have
- Self-host Tailwind CDN script (
iframe-sandbox-csp.ts:37) — Eliminates third-party trust dependency since SRI isn't feasible for JIT runtime. (security, QA) - Move TypeScript transpilation to Web Worker (
transpileReactArtifact.ts:14) — Prevents main-thread blocking for large React artifacts. (performance) - Add CSV row virtualization (
CSVRenderer.tsx:113) — 10K-row CSVs create 10K+ DOM nodes despitecontentVisibility: auto. (performance) - Narrow Zustand selectors in
useArtifactPanel(useArtifactPanel.ts:14) — Select individual fields instead of entire sub-object to reduce drag-resize re-renders. (performance) - Keyboard resize on drag handle (
ArtifactDragHandle.tsx:76) — ARIAseparatorrole should support ArrowLeft/ArrowRight for accessibility. (product, discussion) - Add size-based eviction to content cache (
useArtifactContent.ts:9) — 12 entries × up to 10MB each = potential 120MB in browser memory. (security, performance) - Type
metadataparameter (workspace.py:158) — ReplaceOptional[dict]with a constrainedTypedDictor Pydantic model. (architect, security)
🔵 Nits
- Inline style object (
ArtifactMinimizedStrip.tsx:36) — ExtractwritingMode/textOrientationstyle to a module-level constant. MAX_HISTORYlocation (store.ts:205) — Move from insideopenArtifactaction to module-level next toDEFAULT_PANEL_WIDTH.- File length (
reactArtifactPreview.ts) — At 318 lines, consider extracting the runtime JS template to a separate file.
Human Review Needed
YES — This is a 45-file feature PR touching security-sensitive code (iframe sandboxing, new Function() eval, file upload/download), adding new backend API endpoints, and modifying the copilot tool pipeline. No human approval is currently on record despite 56 commits. The feature flag provides rollback safety, but the security model and test coverage gaps warrant explicit human sign-off.
Risk Assessment
Merge risk: MEDIUM | Rollback: EASY (feature-flagged behind Flag.ARTIFACTS)
CI Status
❌ 4/6 local quality checks failed (frontend lint, typecheck, tests, build all failed to run; backend lint passed, backend tests failed to run). However, GitHub CI shows ✅ 48/48 checks passing — the local failures appear to be environment issues rather than code problems.
Prior Review Issue Tracker
| Issue | Status |
|---|---|
| 🔴 Tailwind CDN without SRI | |
| 🔴 HTML iframe missing CSP / outbound fetch | ✅ Addressed — Documented as accepted risk. connect-src 'none' removal is intentional for dashboard artifact functionality. |
| CSVRenderer weak assertions | not.toThrow(). |
session_id normalization regression test |
|
useArtifactPanel copy-path tests |
|
| Typed exceptions for error dispatch | |
| Keyboard accessibility on drag handle |
| ); | ||
| } | ||
|
|
||
| if (content === null) return null; |
There was a problem hiding this comment.
🤖 🟠 high (security/Unsandboxed iframe)
PDF preview iframe has no sandbox attribute. Malicious PDFs with embedded JS could execute with full capability. The blob URL has a null origin which limits impact, but adding at least a minimal sandbox would improve defense-in-depth.
Suggestion: Consider adding sandbox='allow-scripts allow-same-origin' or testing if modern Chrome/Edge can render PDFs with a sandbox attribute. If not, document the accepted risk in a code comment.
|
|
||
| /** | ||
| * Inject content into the <head> of an HTML document string. | ||
| * If the content has no <head> tag, wraps it in a full document skeleton. |
There was a problem hiding this comment.
🤖 🟠 high (security/CDN without SRI)
Tailwind CDN (cdn.tailwindcss.com/3.4.16) is loaded in every HTML and React artifact iframe without Subresource Integrity. A CDN compromise would inject attacker-controlled JS into all artifact previews. React/ReactDOM scripts use SRI but Tailwind does not.
Suggestion: Since SRI isn't possible for Tailwind's JIT CDN, document this as an accepted risk or consider bundling a static Tailwind CSS file served from same-origin instead of relying on the CDN.
| * | ||
| * What's allowed inside the iframe: | ||
| * - Inline script execution (needed to render React components) | ||
| * - `new Function()` is used to evaluate the compiled code (eval-equivalent) |
There was a problem hiding this comment.
🤖 🟡 medium (security/Outbound network from sandbox)
Sandboxed iframes allow unrestricted outbound fetch/XHR requests. A malicious or AI-hallucinated artifact could perform port scanning of internal networks or make requests to internal services from the user's browser.
Suggestion: Document the accepted risk clearly in the security model comment. Consider whether connect-src restrictions could be applied for non-dashboard artifact types.
| raise fastapi.HTTPException(status_code=409, detail=str(e)) from e | ||
| # write_file raises ValueError for both path-conflict and size-limit | ||
| # cases; map each to its correct HTTP status. | ||
| message = str(e) |
There was a problem hiding this comment.
🤖 🟡 medium (security/Information disclosure)
Raw ValueError messages from write_file are returned to the client in HTTP responses. These could leak internal path structures or implementation details.
Suggestion: Map ValueError messages to generic user-facing error messages rather than passing internal error strings through.
| path: Optional[str] = None, | ||
| mime_type: Optional[str] = None, | ||
| overwrite: bool = False, | ||
| metadata: Optional[dict] = None, |
There was a problem hiding this comment.
🤖 🟢 low (security/Untyped metadata parameter)
The metadata parameter accepts an arbitrary Optional[dict]. While currently only set to known values ('user-upload', 'agent-created'), a future caller could pass unvalidated user input that gets stored in the database.
Suggestion: Consider typing metadata with a constrained Pydantic model (e.g., WorkspaceFileMetadata with an 'origin' enum field) to prevent arbitrary data injection.
| } | ||
| }; | ||
| }, [artifact.id]); | ||
|
|
There was a problem hiding this comment.
🤖 🟢 low (ui-reviewer/Scroll restore timing)
Scroll position save in the cleanup function of useEffect captures scrollRef.current at cleanup time. If React re-renders and replaces the DOM node before the cleanup runs, the ref may be stale (pointing to the old node that still has the correct scrollTop) or null.
Suggestion: This is a minor edge case — the current approach works in practice because React doesn't unmount the scroll div between artifact switches, only when the panel closes. No change needed unless bugs surface.
| a.download = safeName; | ||
| document.body.appendChild(a); | ||
| a.click(); | ||
| a.remove(); |
There was a problem hiding this comment.
🤖 🟢 low (ui-reviewer/Blob URL cleanup race)
URL.revokeObjectURL is called synchronously after a.click(). On slow machines or with very large files, the browser may not have started the download before the blob URL is revoked, causing a failed download.
Suggestion: Use setTimeout(() => URL.revokeObjectURL(url), 10000) to give the browser time to initiate the download before revoking.
| ) as HTMLElement | null; | ||
| startWidthRef.current = panel?.offsetWidth ?? DEFAULT_PANEL_WIDTH; | ||
|
|
||
| setIsDragging(true); |
There was a problem hiding this comment.
🤖 🟢 low (ui-reviewer/Drag handle width constant coupling)
startWidthRef reads from the closest [data-artifact-panel] element's offsetWidth. If the panel's style.width and actual offsetWidth differ (e.g., due to border-box vs content-box or the border-l), the drag could start from a slightly wrong baseline.
Suggestion: Minor — the 1px border difference is negligible. No change needed.
|
|
||
| // Pinned to a specific version to reduce exposure to unannounced upstream | ||
| // changes (SRI is not possible because the JIT runtime is generated on demand). | ||
| export const TAILWIND_CDN_URL = "https://cdn.tailwindcss.com/3.4.16"; |
There was a problem hiding this comment.
🤖 🟡 medium (ui-reviewer/Tailwind CDN version pinning)
Tailwind CDN is pinned to 3.4.16 but loaded without SRI (comment says SRI not possible for JIT runtime). This means a CDN compromise could inject malicious code into every artifact iframe. The sandbox mitigates impact but doesn't eliminate it entirely.
Suggestion: Document the risk acceptance. Consider self-hosting the Tailwind CDN script as a static asset to eliminate the third-party dependency.
|
|
||
| // Fallback: plain text | ||
| return ( | ||
| <pre className="whitespace-pre-wrap break-words p-4 font-mono text-sm text-zinc-800"> |
There was a problem hiding this comment.
🤖 🟢 low (ui-reviewer/Missing error boundary in ArtifactContent)
ArtifactContent wraps ArtifactContentLoader in Suspense but not in an error boundary. If a renderer throws synchronously (e.g., JSON.parse in the json branch, or the globalRegistry.getRenderer call), the error propagates to the nearest parent error boundary rather than showing in-panel.
Suggestion: Wrap ArtifactContentLoader in a React error boundary to keep errors contained within the artifact panel.
There was a problem hiding this comment.
📋 Automated Review — PR #12629
PR #12629 — feat(platform): add copilot artifact preview panel
Author: ntindle | Files: 45
🎯 Verdict: REQUEST_CHANGES
PR Description Quality
✅ Has Why + What + How — PR describes the artifact preview panel feature, file type classification, rendering pipeline, and backend workspace API enhancements.
What This PR Does
Adds a dedicated artifact preview panel to the copilot UI that can render workspace files (HTML, CSV, PDF, code, React/TSX) inline with proper sandboxing. The backend gains paginated file listing, upload metadata tracking (origin: "user-upload" vs "agent-created"), and proper HTTP error codes (409/413). The entire feature is gated behind a Flag.ARTIFACTS feature flag for safe incremental rollout.
Prior Review Status
The previous review requested changes. Status of prior critical/high findings:
⚠️ Tailwind CDN without SRI (iframe-sandbox-csp.ts:33) — Still unaddressed. Tailwind JIT script loaded without integrity hash while React/ReactDOM have SRI pinning. The code comment acknowledges SRI isn't feasible for JIT, but the asymmetry remains.⚠️ No CSP on HTML artifact iframes (ArtifactContent.tsx:119) — Still unaddressed.sandbox="allow-scripts"withoutconnect-srcCSP allows unrestricted outbound fetch/XHR. Documented as accepted trade-off iniframe-sandbox-csp.tsbut the risk (browser-side SSRF from AI-generated HTML) persists.
Specialist Findings
🛡️ Security allow-scripts without allow-same-origin), with well-documented threat model in iframe-sandbox-csp.ts. React/ReactDOM have SRI hashes. Download filename sanitization is thorough. Session ID normalization prevents cross-session leakage.
- 🟡 Tailwind CDN without SRI (
iframe-sandbox-csp.ts:33) — a CDN compromise injects JS into every preview. Self-hosting a pinned build would close this gap. (Flagged by: security, architect — 2) - 🟡 No CSP
connect-srcon sandbox iframes (ArtifactContent.tsx:119) — AI-generated artifacts can make arbitrary outbound requests from the user's network position. Well-documented trade-off. (Flagged by: security, architect — 2) - 🔵
list_workspace_filesexposes internal session path structure (routes.py:380).
🏗️ Architecture ✅ — Clean component boundaries following ComponentName/useComponentName/helpers pattern. Feature flag gating is correct. Zustand store integration is co-located in existing useCopilotUIStore. Dynamic import keeps bundle lean.
- 🟡 Explicit renderer bypass for code/JSON/CSV in
ArtifactContent.tsx:148creates implicit coupling to registry priority ordering. Consider aforceRendererregistry API option. - 🟡
list_filesroute passeslimit/offset/include_all_sessionskwargs (routes.py:367) — verifyWorkspaceManager.list_files()signature accepts these parameters or this will TypeError at runtime. (Flagged by: architect — 1) - 🔵
ChatContaineruses arrow function export, violating the function declaration convention per AGENTS.md — pre-existing, not introduced by this PR.
⚡ Performance ✅ — Content cache bounded at 12 entries, artifact history capped at 25, pagination uses limit+1 trick. Acceptable for client-side UI workload.
- 🟠 TypeScript transpilation runs on main thread with no result caching (
transpileReactArtifact.ts:10) — re-opening the same React artifact re-transpiles identical source (100-500ms blocking). Add a source-keyed cache. (Flagged by: performance — 1) - 🟡 Content cache is count-bounded (12 entries) but not byte-bounded (
useArtifactContent.ts:9). With 10MB artifacts, cache can hold ~120MB of strings. - 🔵
useAutoOpenArtifactsallocatesnew Set(messages.map(...))on every streaming chunk (useAutoOpenArtifacts.ts:75).
🧪 Testing
- 🟠 CSVRenderer tests are smoke-only (
CSVRenderer.test.ts:49-66) — all five render tests assert onlynot.toThrow()without verifying parsed cell content. A parser bug silently dropping fields would pass. (Flagged by: testing — 1) - 🟠
useArtifactPanelhook has zero tests (useArtifactPanel.ts:13) — this is the main orchestration hook wiring store to UI (Escape key handling, copy with cache fallback, viewport-responsive width clamping). (Flagged by: testing — 1) - 🟠 No negative test for empty-string
session_idnormalization (routes.py:361) — the route normalizes""→Nonebut no test sendssession_id=""to verify. (Flagged by: testing — 1) - 🟡 Backend
soft_deletefailure path (routes.py:280) and copilot toolmetadata={"origin": "agent-created"}(workspace_files.py:848) lack tests.
📖 Quality ✅ — Clean Tailwind usage, good JSDoc on key interfaces, thorough security rationale documentation. Readability: A.
- 🟠
reactArtifactPreview.tsis 318 lines with a 260-line template literal containing embedded JS with no IDE support/linting (reactArtifactPreview.ts:56). Exceeds 200-line frontend guideline. Split into composable functions. (Flagged by: quality — 1) - 🔵 Origin badge color logic duplicated between
ArtifactCard.tsx:88andArtifactPanelHeader.tsx:92. - 🔵
MAX_HISTORY = 25defined inline in action body (store.ts:205) rather than at module scope. - 🔵 Logger uses f-string with
{e}losing traceback (routes.py:279); useexc_info=True.
📦 Product ✅ — Complete feature implementation with safe defaults (download-only for unknown types), proper error states with retry, cached copy for instant feel, and mobile Sheet overlay.
- 🔵 Drag handle missing
aria-valuemin/aria-valuemax/aria-valuenowfor screen readers (ArtifactDragHandle.tsx:77). - 🔵 No loading indicator for PDF/image content (
ArtifactContent.tsx:82, 98). - 🔵 Minimized strip vertical text truncates with no tooltip (
ArtifactMinimizedStrip.tsx:33).
📬 Discussion
- 🟠 @0ubbe's concern about hook-level tests being too implementation-coupled (preferring component-level integration tests) was never acknowledged by the author.
- 🔵 @majdyz's pagination blocker thread shows author saying "not fixing" but the code actually implements pagination — thread should be updated.
- 🔵 Auto-open first-artifact-in-session edge case deferred with no tracking issue created.
🔎 QA ✅ — All 20 test scenarios passed. Backend CRUD, pagination, error codes (401/404/409/413), path traversal sanitization, session_id normalization, and auth enforcement all verified via API calls. All 55 unit test files pass. Feature flag gating confirmed (panel hidden without Flag.ARTIFACTS). Full UI testing blocked by missing LLM API key (pre-existing env issue).
🟠 Should Fix
- CSVRenderer tests need actual value assertions (
CSVRenderer.test.ts:49-66) — Replacenot.toThrow()smoke tests with assertions on parsed headers and cell content. A parser regression silently producing wrong data would pass current tests. (Flagged by: testing — 1) - Add
useArtifactPanelhook tests (useArtifactPanel.ts:13) — Cover Escape key handling (with dialog-detection guard), copy with cache-vs-fetch fallback, and viewport-responsiveeffectiveWidthclamping. This is the central wiring hook with no coverage. (Flagged by: testing — 1) - Add empty-string
session_idnormalization test (routes.py:361) — Sendsession_id=""and assert it behaves identically tosession_id=None. The normalization exists but is untested. (Flagged by: testing — 1) - Cache TypeScript transpilation results (
transpileReactArtifact.ts:10) — Add aMap<string, string>keyed by source content to skip re-transpilation of identical source. Currently blocks main thread for 100-500ms on every re-open. (Flagged by: performance — 1) - Respond to @0ubbe's testing pattern concern — Acknowledge or address the feedback about hook-level vs. component-level integration tests. Reviewer concerns should not be silently ignored. (Flagged by: discussion — 1)
🟡 Nice to Have
- Self-host Tailwind or use pre-built CSS CDN with SRI (
iframe-sandbox-csp.ts:33) — Closes the SRI gap, but the JIT runtime makes this complex. (security, architect) - Add CSP
connect-srcto sandbox iframes (ArtifactContent.tsx:119) — Would restrict outbound requests from AI-generated HTML. Complex to implement without breaking legitimate previews. (security, architect) - Split
buildReactArtifactSrcDocinto composable functions (reactArtifactPreview.ts:56) — 260-line template literal is hard to maintain. ExtractbuildRequireShim,buildErrorBoundary,buildRenderBootstrap. (quality) - Byte-bounded content cache (
useArtifactContent.ts:9) — Track total cached bytes rather than just entry count to prevent 120MB memory pressure. (performance) - Add
referrerpolicy="no-referrer"to PDF iframe (ArtifactContent.tsx:98) — Defense-in-depth for the unsandboxed PDF iframe. (architect)
🔵 Nits
- Duplicated origin badge logic (
ArtifactCard.tsx:88,ArtifactPanelHeader.tsx:92) — Extract a sharedgetOriginBadgeClasses(origin)helper. - Inline
MAX_HISTORY(store.ts:205) — Move to module scope alongsideDEFAULT_PANEL_WIDTH. - Logger f-string loses traceback (
routes.py:279) — Uselogger.warning(..., exc_info=True)instead off"...{e}". - Drag handle missing ARIA value attributes (
ArtifactDragHandle.tsx:77) — Addaria-valuemin/aria-valuemax/aria-valuenow. - Minimized strip tooltip (
ArtifactMinimizedStrip.tsx:33) — Addtitleattribute for truncated vertical text.
QA Screenshots
| Screenshot | Description |
|---|---|
|
Copilot page renders correctly ✅ |
 |
Artifact panel correctly hidden when Flag.ARTIFACTS is off ✅ |
Human Review Needed
YES — 45 files changed across frontend and backend, new iframe sandboxing with security implications, new API endpoints, and the testing pattern disagreement between author and reviewer needs human resolution.
Risk Assessment
Merge risk: MEDIUM | Rollback: EASY (feature flag Flag.ARTIFACTS defaults to false)
CI Status
❌ Local quality checks show failures in lint, typecheck, test, and build — however CI reports 48/48 required GitHub checks passing. Local failures appear to be environment-related (missing generated API client, missing dependencies). Frontend coverage at 38.72% is below the 80% target but Codecov gates are passing.
| const wrapped = wrapWithHeadInjection(content, tailwindScript); | ||
| return ( | ||
| <iframe | ||
| sandbox="allow-scripts" |
There was a problem hiding this comment.
🤖 🟡 medium (security/Network isolation)
HTML artifact iframe allows arbitrary outbound network requests (fetch/XHR) from the user's network position. AI-generated or malicious HTML artifacts could scan internal networks or reach localhost services.
Suggestion: Consider adding a restrictive CSP meta tag for connect-src to limit outbound requests to known-safe origins, or document the SSRF-from-browser risk as an accepted trade-off in a threat model.
|
|
||
| // Pinned to a specific version to reduce exposure to unannounced upstream | ||
| // changes (SRI is not possible because the JIT runtime is generated on demand). | ||
| export const TAILWIND_CDN_URL = "https://cdn.tailwindcss.com/3.4.16"; |
There was a problem hiding this comment.
🤖 🟡 medium (security/Supply chain / SRI)
Tailwind CDN script (cdn.tailwindcss.com/3.4.16) is loaded without Subresource Integrity (SRI), unlike React/ReactDOM which have SRI hashes. A CDN compromise would inject arbitrary JS into every artifact preview.
Suggestion: If SRI is infeasible for the JIT runtime, consider self-hosting a pinned Tailwind build or using the pre-built CSS CDN (which supports SRI) instead of the JIT script.
| files=[ | ||
| WorkspaceFileItem( | ||
| id=f.id, | ||
| name=f.name, |
There was a problem hiding this comment.
🤖 🟡 medium (security/Information disclosure)
ListFilesResponse exposes the internal file path (e.g., /sessions/sess-123/hello.txt) which reveals session ID structure and storage layout to the frontend API consumer.
Suggestion: Consider omitting the path field from the response or replacing it with a sanitized display name that doesn't expose internal session scoping.
| // (Chromium bug #413851). The blob URL has a null origin so it can't | ||
| // access the parent page regardless. | ||
| return ( | ||
| <iframe src={pdfUrl} className="h-full w-full" title={artifact.title} /> |
There was a problem hiding this comment.
🤖 🟢 low (security/Sandbox gap)
PDF preview iframe has no sandbox attribute due to Chromium bug #413851. While blob URLs have null origin, the iframe has full capabilities (forms, popups, scripts if the PDF triggers them).
Suggestion: Add a comment-level security note and consider adding sandbox="allow-scripts allow-same-origin" if Chromium fixes the bug in the future, or verify that blob PDFs can't trigger navigation.
| // Inject Tailwind CDN — no CSP (see iframe-sandbox-csp.ts for why) | ||
| const tailwindScript = `<script src="${TAILWIND_CDN_URL}"></script>`; | ||
| const wrapped = wrapWithHeadInjection(content, tailwindScript); | ||
| return ( |
There was a problem hiding this comment.
🤖 🟡 medium (architect/Security: PDF iframe unsandboxed)
PDF iframe intentionally omits sandbox attribute (Chrome PDF bug), but also lacks referrerpolicy="no-referrer" — the blob URL iframe can leak the parent page URL via Referer header if the PDF triggers navigation.
Suggestion: Add referrerpolicy="no-referrer" to the PDF iframe as defense-in-depth.
| @@ -0,0 +1,141 @@ | |||
| import { beforeEach, describe, expect, it } from "vitest"; | |||
There was a problem hiding this comment.
🤖 🟡 medium (discussion/Unresolved reviewer concern)
@0ubbe flagged that store unit tests are too tightly coupled to implementation and the team prefers component-level integration tests. No response from author.
Suggestion: Acknowledge the concern and either convert to component-level tests per team convention or explain why hook-level tests are appropriate here.
| @@ -0,0 +1,140 @@ | |||
| import { act, renderHook } from "@testing-library/react"; | |||
There was a problem hiding this comment.
🤖 🟡 medium (discussion/Unresolved reviewer concern)
@0ubbe recommended testing hooks via component tests in a tests/main.test.tsx pattern rather than isolated hook tests. No direct response from author.
Suggestion: Respond to @0ubbe's comment and align on testing strategy: either adopt the component-level pattern or explain the deviation.
| used_percent=round((used_bytes / limit_bytes) * 100, 1) if limit_bytes else 0, | ||
| file_count=file_count, | ||
| ) | ||
|
|
There was a problem hiding this comment.
🤖 🟢 low (discussion/Stale discussion thread)
@majdyz's pagination blocker was marked 'not fixing' by the author, but the final code DOES include limit/offset/has_more. The thread is misleading.
Suggestion: Update the reply to @majdyz's blocker comment confirming pagination was implemented, so the thread accurately reflects the current state.
| useEffect(() => { | ||
| messageFingerprintsRef.current = new Map(); | ||
| hasInitializedRef.current = false; | ||
| }, [sessionId]); |
There was a problem hiding this comment.
🤖 🟢 low (discussion/Deferred follow-up)
Author acknowledged CodeRabbit's concern that auto-open may skip the first artifact in a new session and said 'will investigate'. No follow-up or tracking issue created.
Suggestion: Either fix the hydration vs. live-message edge case or create a tracking issue so it doesn't get lost.
| <th | ||
| key={i} | ||
| className="px-3 py-2 text-left font-medium text-zinc-700" | ||
| > |
There was a problem hiding this comment.
🤖 🟢 low (discussion/Accessibility gap)
CodeRabbit flagged that CSV column sorting click handlers on elements are not keyboard-accessible. No response from author.
Suggestion: Use inside or add onKeyDown/tabIndex for keyboard sorting support.
There was a problem hiding this comment.
📋 Automated Review — PR #12629
PR #12629 — feat(platform): add copilot artifact preview panel
Author: ntindle | Files: 45
🎯 Verdict: REQUEST_CHANGES
PR Description Quality
✅ Has Why + What + How — PR describes the artifact preview panel feature, classification system, renderer pipeline, and backend workspace file API additions.
What This PR Does
Adds a copilot artifact preview panel that automatically detects workspace files (HTML, CSV, PDF, images, React/JSX, code) generated during copilot conversations, classifies them by type, and renders inline previews in a resizable side panel. The backend gains a new list_workspace_files endpoint with pagination and origin metadata tracking. The entire feature is gated behind a Flag.ARTIFACTS feature flag for safe rollout.
Specialist Findings
🛡️ Security allow-scripts without allow-same-origin), preventing session hijacking and parent DOM access. However, sandboxed iframes retain full outbound network capability.
- 🟠 Sandboxed HTML/React iframes can make arbitrary
fetch/XHRrequests, enabling internal network scanning from the user's browser via prompt-injected artifacts (ArtifactContent.tsx:119,reactArtifactPreview.ts:263). Document as accepted risk or addconnect-srcCSP restrictions. - 🟡 PDF iframe lacks
sandboxattribute (ArtifactContent.tsx:98) — safe due to blob URL null origin but missing safety invariant documentation. - 🟡 Tailwind CDN loaded without SRI integrity (
iframe-sandbox-csp.ts:33) — version-pinned but CDN compromise risk exists; sandbox mitigates blast radius.
🏗️ Architecture ✅ — Clean separation of concerns: pure classification function, pluggable renderers via type dispatch, Zustand state management following project patterns, proper feature flag gating, and lazy-loaded panel via next/dynamic.
- 🟠
list_workspace_fileslacks explicitORDER BY(routes.py:368), making pagination non-deterministic — pages may skip or duplicate rows. (Flagged by: architect — 1) - 🟡 Module-level
contentCacheMap bypasses React's data flow (useArtifactContent.ts:14) — acceptable with the 12-entry cap but harder to reason about for SSR/tests. - 🟡
metadataparameter typed asOptional[dict](workspace.py:158) — aTypedDictwould prevent undocumented key accumulation.
⚡ Performance ✅ — Content cache is bounded, pagination is capped, and the TypeScript compiler is lazy-loaded. Minor efficiency concerns in streaming hot paths.
- 🟠
extractWorkspaceArtifactscompiles newRegExpobjects per workspace URI match inside a loop that runs on every SSE streaming chunk (helpers.ts:271). Pre-compile patterns outside the loop. - 🟡 Content cache limits entries (12) but not bytes (
useArtifactContent.ts:9) — 12 large artifacts could hold 60MB+. Consider a byte-budget cap. - 🟡 TypeScript compiler (~5MB) dynamic import causes noticeable cold-start on first React artifact preview (
transpileReactArtifact.ts:14). Consider prefetching on panel mount. - 🟡 No row-count limit or virtualization for CSV tables (
CSVRenderer.tsx:113) — 10K+ row CSVs will create thousands of DOM nodes.
🧪 Testing
- 🟠
resolveWorkspaceUrls()has zero test coverage despite complex regex with negative lookbehinds that transforms user-visible markdown links (helpers.ts:212). (Flagged by: testing — 1) - 🟠
getMessageArtifacts()— untested aggregation with deduplication; bugs silently drop or duplicate artifacts (helpers.ts:178). (Flagged by: testing — 1) - 🟠 CSVRenderer render tests only assert
not.toThrow()— cannot catch parsing regressions like BOM stripping or quoted field handling (CSVRenderer.test.ts:46). Verify actual cell values. (Flagged by: testing — 1) - 🟠 No test verifies auto-open is gated behind
Flag.ARTIFACTSfeature flag (useAutoOpenArtifacts.ts:1). If the gate breaks, artifacts auto-open for users who shouldn't see them. (Flagged by: testing — 1) - 🟡
parseSpecialMarkers(),buildRenderSegments(),splitReasoningAndResponse()— untested regex-heavy and branching functions (helpers.ts:68-148). - 🟡 Backend empty-string
session_id=""normalization has no dedicated test (routes_test.py), though QA manually verified it works.
📖 Quality ✅ — Readability score A. Consistent naming, proper component structure, security decisions well-documented. Minor magic number and duplication issues.
- 🟡
ArtifactRendereris 124 lines with an if-chain (ArtifactContent.tsx:66) — consider a renderer map pattern. - 🔵 Magic numbers
320/0.85duplicated betweenArtifactDragHandle.tsx:17andstore.ts:65-66— extract shared constants. - 🔵
MAX_HISTORY = 25andCONTENT_CACHE_MAX = 12lack rationale comments (store.ts:205,useArtifactContent.ts:9).
📦 Product
- 🟡 Auto-open has no user opt-out toggle (
useAutoOpenArtifacts.ts:88) — may be disruptive for power users. - 🟡 Drag handle (
ArtifactDragHandle.tsx:79) hasrole="separator"but no keyboard resize interaction (arrow keys). WCAG requires keyboard operability for interactive separators. (Flagged by: product — 1) - 🟡 Image artifacts have no
onErrorhandler (ArtifactContent.tsx:84) — broken URLs show browser default broken image icon. - 🟡 No
ErrorBoundarywrapsArtifactContentLoader(ArtifactContent.tsx:192) — a malformed artifact could crash the copilot page.
📬 Discussion
- 🟠
useAutoOpenArtifactssession-reset bug: first artifact in a new session is skipped (treated as hydration). Acknowledged by author but deferred without a tracking issue. - 🟡 @0ubbe's feedback about hook-level tests being too implementation-coupled vs project convention of component-level integration tests was not directly addressed.
⚠️ No formal human code review approval exists despite 56 commits across 45+ files.reviewDecision: APPROVEDappears to come from auto-approval, not an individual reviewer.
🔎 QA ✅ — Comprehensive API testing: 14 scenarios covering workspace file CRUD, pagination, session scoping, auth rejection, duplicate uploads (409), and storage usage. 110 unit tests pass (88 frontend + 22 backend).
- Limitation: Artifact preview panel UI could not be exercised because
Flag.ARTIFACTSis disabled in the test environment. Backend logic fully validated.
🟠 Should Fix
resolveWorkspaceUrlsneeds tests (ChatMessagesContainer/helpers.ts:212) — Complex regex with negative lookbehinds, video MIME special-casing, and URL construction. Zero coverage for a function that transforms all workspace file links visible to users. (Flagged by: testing)getMessageArtifactsneeds tests (ChatMessagesContainer/helpers.ts:178) — Untested aggregation with deduplication. A bug here silently drops or duplicates artifacts. (Flagged by: testing)- CSVRenderer render tests need value assertions (
CSVRenderer.test.ts:46) — Five render tests only assertnot.toThrow(). Verify actual parsed cell values, especially for BOM stripping and quoted fields. (Flagged by: testing) - Feature flag gate test for auto-open (
useAutoOpenArtifacts.ts) — No test verifiesFlag.ARTIFACTS = falseprevents auto-open. This is a security-adjacent gate. (Flagged by: testing) - Add explicit ordering to
list_workspace_files(routes.py:368) — Pagination without deterministicORDER BYcan skip or duplicate rows across pages. (Flagged by: architect) - Pre-compile regex in
extractWorkspaceArtifacts(helpers.ts:271) — New RegExp compiled per URI match on every streaming chunk is O(n) compilations in a hot path. Move pattern compilation outside the loop. (Flagged by: performance) - Track session-reset auto-open bug (
useAutoOpenArtifacts.ts:31) — First artifact in a new session is skipped. Create a follow-up issue or fix in this PR. (Flagged by: discussion)
🟡 Nice to Have
- Document sandbox network access as accepted risk (
ArtifactContent.tsx:119,reactArtifactPreview.ts:263) — Outboundfetch/XHRfrom sandboxed iframes enables internal network probing. Considerconnect-srcCSP for non-dashboard previews, or document in threat model. (security) - Add byte-budget cap to content cache (
useArtifactContent.ts:9) — Entry count is capped at 12 but total bytes are unbounded; 12 large artifacts could hold 60MB+. (performance) - Prefetch TypeScript compiler on panel mount (
transpileReactArtifact.ts:14) — ~5MB dynamic import causes cold-start lag on first React preview. (performance) - Add keyboard resize to drag handle (
ArtifactDragHandle.tsx:79) —role="separator"should support ArrowLeft/ArrowRight for WCAG compliance. (product) - Add ErrorBoundary around ArtifactContentLoader (
ArtifactContent.tsx:192) — Malformed artifacts could crash the entire copilot page. (product) - Type
metadataas TypedDict (workspace.py:158) — Prevents undocumented key accumulation as the feature grows. (architect) - Extract inline JS runtime (
reactArtifactPreview.ts:49) — ~200 lines of vanilla JS in a template literal is hard to maintain/lint independently. (quality, architect) - Add tests for
parseSpecialMarkers,buildRenderSegments,splitReasoningAndResponse(helpers.ts:68-148) — Untested regex-heavy functions in the chat rendering pipeline. (testing)
🔵 Nits
- Extract shared panel width constants (
ArtifactDragHandle.tsx:17,store.ts:65) —320and0.85duplicated as inline literals. - Add rationale comments for magic numbers (
store.ts:205,useArtifactContent.ts:9) —MAX_HISTORY = 25andCONTENT_CACHE_MAX = 12are unexplained. - Deduplicate HTML iframe pattern (
ArtifactContent.tsx:113,HTMLRenderer.tsx:13) — Same Tailwind CDN injection + sandbox + srcDoc pattern in two places.
QA Screenshots
| Screenshot | Description |
|---|---|
 |
Copilot page loads successfully with chat UI; artifact panel area present but feature-flagged off ✅ |
Human Review Needed
YES — This is a 56-commit, 45-file PR introducing iframe-based code execution (sandboxed), new backend file API endpoints, and complex streaming-path logic. No formal human code review approval exists despite the security surface area. A human reviewer should validate the sandbox model and the backend authorization scoping.
Risk Assessment
Merge risk: MEDIUM | Rollback: EASY (feature flag Flag.ARTIFACTS gates the entire feature)
CI Status
❌ 2/6 local quality checks passed. Frontend lint, typecheck, build, and tests failed — but these appear to be environment/setup issues (missing generated API client). QA specialist confirmed all 110 unit tests pass after proper setup. CI on GitHub shows ✅ 48/48 checks passing.
There was a problem hiding this comment.
📋 Automated Review — PR #12629
PR #12629 — feat(platform): add copilot artifact preview panel
Author: ntindle | Files: 45
🎯 Verdict: REQUEST_CHANGES
PR Description Quality
✅ Has Why + What + How — PR describes the artifact preview panel feature, rendering pipeline, backend workspace file endpoints, and feature flag gating.
What This PR Does
Adds a dedicated artifact preview panel to the copilot interface that can render AI-generated files (HTML, React/TSX, CSV, PDF, code, images) inline with classification-based routing, auto-open behavior, resize/minimize/maximize controls, and download/copy actions. Backend changes add workspace file listing with pagination, metadata origin tracking (user-upload vs agent-created), and improved error mapping (409 vs 413). The entire feature is gated behind a Flag.ARTIFACTS feature flag for safe rollout.
Specialist Findings
🛡️ Security allow-scripts without allow-same-origin), with proper </script> breakout escaping and SRI on React/ReactDOM CDN. However, sandboxed iframes still permit outbound network requests, meaning prompt-injected HTML/React artifacts could exfiltrate artifact content to external servers. PDF rendering is unsandboxed due to a Chromium bug. All risks are documented and accepted.
- 🟡 Outbound network from HTML/React iframes (
ArtifactContent.tsx:119,reactArtifactPreview.ts:263) — sandbox prevents parent-page access butfetch()/XHRto external servers remains possible. Blast radius is artifact content only (no cookies/tokens). Well-documented trade-off. - 🟡 Unsandboxed PDF iframe (
ArtifactContent.tsx:98) — Chromium bug prevents sandboxed PDF rendering. Blob URL provides null-origin isolation but no sandbox containment against PDF renderer exploits. - 🟡 Raw
stylesMarkupinterpolation (reactArtifactPreview.ts:62) — currently safe (hardcoded input), but function signature accepts any string. A@internalannotation or branded type would prevent future misuse.
🏗️ Architecture ✅ — Clean component decomposition following repo conventions (ComponentName/ComponentName.tsx + hook + helpers). Zustand store integration is well-typed. Feature flag gating is correct. Dynamic import for the client-only panel is appropriate.
- 🟠 Content cache not cleared on session switch (
useArtifactContent.ts:14) — module-levelcontentCachepersists across session transitions.clearContentCache()is only called fromclearCopilotLocalData, not on session changes. (Flagged by: architect, performance — 2) - 🟡 Untyped
metadatadict on backend (workspace.py:158) —Optional[dict]with no schema validation; aFileMetadataTypedDict would prevent drift as more fields are added. - 🔵 Barrel file (
OutputRenderers/index.ts) violates repo guideline "No barrel files or index.ts re-exports."
⚡ Performance ✅ — Size gates (10MB), 12-entry content cache, and streaming-aware fingerprinting show good performance awareness. No blocking issues.
- 🟡 TypeScript compiler import not cached (
transpileReactArtifact.ts:14) —import("typescript")called on every transpile; caching the promise would avoid repeated overhead for the ~5MB bundle. - 🟡 Content cache unbounded per-entry (
useArtifactContent.ts:8) — 12 entries × up to 10MB each = theoretical 120MB in a module-level Map. A total-bytes cap would be safer. (Flagged by: architect, performance — 2) - 🔵 CSV parser uses char-by-char string concatenation (
CSVRenderer.tsx:22) — O(n²) intermediate strings for large files; index-based slicing would be more efficient.
🧪 Testing
- 🟠
useArtifactPanelhook has zero tests (useArtifactPanel.ts:13) — orchestrates copy (cache-hit vs fetch), download with error toasts, Escape key handling, viewport clamping, and source-view reset. (Flagged by: testing, discussion — 2) - 🟠
ArtifactContentrenderer routing untested (ArtifactContent.tsx:78) — the branching logic across 8+ render paths (HTML/React/Code/JSON/CSV/image/PDF/fallback) has no regression test, yet this is where the "Python rendered as markdown" bug lived. - 🟠 Backend empty-string
session_idnormalization untested (routes.py:218,routes.py:368) — bothupload_fileandlist_workspace_filesnormalizesession_id = session_id or Nonebut no test sendssession_id="". - 🟠 Backend
metadata={"origin": "agent-created"}untested (workspace_files.py:848) — if the kwarg is dropped, agent-created files lose origin tagging. - 🟡 CSVRenderer tests are smoke-only (
CSVRenderer.test.ts:48) — fivenot.toThrow()assertions without checking parsed output. A null-returning parser would pass all tests. - 🟡 Frontend diff coverage at 38.72% vs 80% target.
📖 Quality ✅ — Readability is strong (grade A). Naming is clear and self-descriptive. Security model is thoroughly documented in iframe-sandbox-csp.ts. Phosphor Icons used throughout.
- 🔵 Duplicated HTML sandbox injection between
ArtifactContent.tsx:128andHTMLRenderer.tsx:14— extract sharedbuildHtmlPreviewSrcDoc(). - 🔵 Magic values (
MAX_HISTORY=25,minWidth=320,maxWidthPercent=85) scattered across store/components instead of shared constants. - 🔵
reactArtifactPreview.tsat 318 lines exceeds the 200-line frontend guideline.
📦 Product ✅ — Feature-complete against claimed scope. Good UX details: history management with ping-pong detection, skeleton loading states, mobile Sheet overlay, keyboard dismiss via Escape.
- 🟡 Auto-open has no user opt-out (
useAutoOpenArtifacts.ts:87) — panel steals viewport space during active typing with no preference toggle. - 🟡 10MB download-only gate has no user-facing explanation (
helpers.ts:203) — users may wonder why large artifacts can't be previewed. - 🔵
SourceTogglemissingrole="group"wrapper for screen readers (SourceToggle.tsx:12).
📬 Discussion ✅ — All ~60+ review comments from @majdyz, @coderabbitai, @cursor, and @sentry have been addressed across 56 commits. No unresolved threads remain. CI is fully green (48/48 checks passing).
🔎 QA ✅ — Comprehensive API-level testing of all backend endpoints (list/upload/download/delete) with pagination, session scoping, metadata tracking, error codes (409/413), and auth enforcement. All 22 backend + 948 frontend unit tests pass. Artifact panel correctly gated behind feature flag.
🟠 Should Fix
-
useArtifactPanelhook needs test coverage (useArtifactPanel.ts:13) — This is the main orchestration hook binding store actions to UI. Copy (cache-hit vs fetch fallback), download error handling, Escape key, and viewport clamping are all untested. (Flagged by: testing, discussion — 2) -
ArtifactContentrenderer routing needs regression tests (ArtifactContent.tsx:78) — The 8+ branch dispatch (HTML/React/Code/JSON/CSV/image/PDF/fallback) is where the "Python rendered as markdown" bug lived. Without a test, this exact class of bug can silently regress. (Flagged by: testing) -
Empty-string
session_idnormalization needs tests (routes.py:218,routes.py:368) — Thesession_id or Nonenormalization is a correctness invariant. A test sendingsession_id=""to both upload and list endpoints would prevent regression. (Flagged by: testing) -
Content cache should clear on session switch (
useArtifactContent.ts:14) — Module-level cache persists across sessions. AddclearContentCache()to the session-transition effect to prevent stale cross-session hits. (Flagged by: architect, performance — 2)
🟡 Nice to Have
- Cache TypeScript compiler import (
transpileReactArtifact.ts:14) —let tsPromise; function getTS() { return tsPromise ??= import('typescript'); }avoids repeated promise overhead for the ~5MB bundle. (performance) - Add total-bytes cap to content cache (
useArtifactContent.ts:8) — 12 entries × 10MB = theoretical 120MB. A cumulative byte threshold with LRU eviction would bound memory. (architect, performance) - Backend
FileMetadatatyped model (workspace.py:158) — ReplaceOptional[dict]with a Pydantic model to validate metadata shape and prevent drift. (architect) - CSVRenderer tests should verify parsed output (
CSVRenderer.test.ts:48) — At minimum, the "embedded newline" test should assert the field was parsed as a single cell. (testing) - Auto-open user preference toggle (
useAutoOpenArtifacts.ts:87) — Allow users to disable auto-open to prevent viewport disruption during active typing. (product)
🔵 Nits
- Duplicated HTML sandbox injection (
ArtifactContent.tsx:128,HTMLRenderer.tsx:14) — Extract sharedbuildHtmlPreviewSrcDoc()helper. - Magic values scattered (
store.ts:205,ArtifactDragHandle.tsx:73) — LiftMAX_HISTORY,minWidth,maxWidthPercentto shared constants. - Barrel file (
OutputRenderers/index.ts) — Repo guidelines say no barrel files. SourceToggleaccessibility (SourceToggle.tsx:12) — Wrap in<div role="group" aria-label="View mode">.session_idnormalization duplication (routes.py:219,routes.py:363) — Extract_normalize_session_id()helper.
QA Screenshots
| Screenshot | Description |
|---|---|
 |
Copilot page loads correctly with artifact panel feature-flagged ✅ |
Human Review Needed
YES — This is a 45-file feature addition with security-sensitive iframe sandboxing, arbitrary code execution (React preview via new Function()), new backend API endpoints, and complex frontend state management. Human review should verify the iframe sandbox model and the React preview execution path are acceptable for the threat model.
Risk Assessment
Merge risk: MEDIUM | Rollback: EASY (feature flag Flag.ARTIFACTS gates entire surface)
CI Status
✅ 48/48 CI checks pass. Frontend diff coverage at 38.72% (below 80% target, soft gate). Backend lint ✅, backend tests ✅.
| const wrapped = wrapWithHeadInjection(content, tailwindScript); | ||
| return ( | ||
| <iframe | ||
| sandbox="allow-scripts" |
There was a problem hiding this comment.
🤖 🟠 high (security/Outbound exfiltration from HTML iframe)
HTML artifacts rendered with sandbox="allow-scripts" but no network restriction. AI-generated or prompt-injected HTML can make fetch/XHR calls to attacker-controlled servers, exfiltrating artifact content.
Suggestion: Document this as accepted risk in a security review record. Consider adding an opt-in CSP toggle for security-conscious deployments, or implement a same-origin service-worker proxy that blocks outbound requests from artifact iframes.
| try { | ||
| const exports = {}; | ||
| const module = { exports }; | ||
| const factory = new Function( |
There was a problem hiding this comment.
🤖 🟠 high (security/Arbitrary code execution in React preview)
AI-generated code is executed via new Function() inside a sandboxed iframe. The sandbox prevents parent-page access, but executed code has full network access (fetch/XHR) which could be used for exfiltration or as an attack relay.
Suggestion: Accepted trade-off given sandbox isolation. Consider adding a network-disabled mode or warning indicator when React artifacts make outbound requests.
| // (Chromium bug #413851). The blob URL has a null origin so it can't | ||
| // access the parent page regardless. | ||
| return ( | ||
| <iframe src={pdfUrl} className="h-full w-full" title={artifact.title} /> |
There was a problem hiding this comment.
🤖 🟡 medium (security/Unsandboxed PDF iframe)
PDF artifacts are rendered in an unsandboxed iframe due to Chromium bug #413851. If a crafted PDF exploits a browser PDF renderer vulnerability, there is no sandbox containment. The blob URL null origin provides some isolation.
Suggestion: Consider using pdf.js (Mozilla's JS-based PDF renderer) as an alternative that works within sandboxed iframes, avoiding reliance on the browser's native PDF plugin.
| <head> | ||
| <meta charset="utf-8" /> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1" /> | ||
| <title>${safeTitle}</title> |
There was a problem hiding this comment.
🤖 🟡 medium (security/Raw stylesMarkup injection)
The stylesMarkup parameter is interpolated directly into the HTML template. Currently only called with hardcoded safe output from collectPreviewStyles(), but the function signature accepts any string — a future caller could pass untrusted input.
Suggestion: Add a JSDoc @internal or type-brand annotation marking stylesMarkup as trusted-only, or validate/sanitize the input within buildReactArtifactSrcDoc.
|
|
||
| // Pinned to a specific version to reduce exposure to unannounced upstream | ||
| // changes (SRI is not possible because the JIT runtime is generated on demand). | ||
| export const TAILWIND_CDN_URL = "https://cdn.tailwindcss.com/3.4.16"; |
There was a problem hiding this comment.
🤖 🟢 low (security/Tailwind CDN without SRI)
Tailwind CDN is loaded with a pinned version but without SRI integrity hash. A CDN compromise would affect all HTML and React artifact previews. Comment notes SRI isn't possible for the JIT runtime.
Suggestion: Consider self-hosting the Tailwind CSS build or using a pre-built Tailwind stylesheet with SRI instead of the JIT CDN script.
| sizeBytes?: number, | ||
| ): ArtifactClassification { | ||
| // Size gate: >10MB is download-only regardless of type. | ||
| if (sizeBytes && sizeBytes > TEN_MB) return KIND["download-only"]; |
There was a problem hiding this comment.
🤖 🟢 low (product/UX: Missing size-gate explanation)
Files >10MB are silently classified as 'download-only' with a generic 'File' label. Users who generated a large artifact may not understand why preview is unavailable.
Suggestion: Change the label for the size-gated download-only case to something like 'File (too large to preview)' or add a tooltip on the ArtifactCard when size gating triggers.
|
|
||
| export function SourceToggle({ isSourceView, onToggle }: Props) { | ||
| return ( | ||
| <div className="flex items-center rounded-md border border-zinc-200 bg-zinc-50 p-0.5 text-xs font-medium"> |
There was a problem hiding this comment.
🤖 🟢 low (product/Accessibility: Toggle group)
The Preview/Source toggle uses two independent buttons with aria-pressed but no role='group' or aria-label on the container, making it harder for screen readers to understand these are a mutually exclusive pair.
Suggestion: Wrap the toggle in a <div role='group' aria-label='View mode'> to convey the relationship between the two buttons.
| <iframe src={pdfUrl} className="h-full w-full" title={artifact.title} /> | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
🤖 🟢 low (product/UX: PDF sandbox inconsistency)
PDF iframe has no sandbox attribute, unlike HTML and React previews which use sandbox='allow-scripts'. While the blob: URL provides origin isolation, the inconsistency may surprise security reviewers.
Suggestion: Add a code comment explaining the security posture or add sandbox='allow-scripts allow-same-origin' if Chromium allows PDF rendering with that combination.
| @@ -0,0 +1,125 @@ | |||
| "use client"; | |||
There was a problem hiding this comment.
🤖 🟡 medium (discussion/test coverage gap)
Frontend diff coverage is 38.72% (target 80%). ArtifactPanel, ArtifactCard, ArtifactContent, ArtifactPanelHeader, ArtifactMinimizedStrip, ArtifactDragHandle, and SourceToggle components lack integration tests — only helpers/hooks/store have test files.
Suggestion: Add integration tests for the main ArtifactPanel rendering paths (open/close/minimize/maximize, mobile sheet, artifact type switching) to bring coverage closer to the 80% target.
| }: UseAutoOpenArtifactsOptions) { | ||
| const openArtifact = useCopilotUIStore((state) => state.openArtifact); | ||
| const messageFingerprintsRef = useRef<Map<string, string>>(new Map()); | ||
| const hasInitializedRef = useRef(false); |
There was a problem hiding this comment.
🤖 🟢 low (discussion/deferred follow-up)
CodeRabbit flagged that after sessionId reset, the first artifact in a new session may be treated as hydration and skipped. Author acknowledged and said 'Will investigate' but no fix was committed.
Suggestion: Track this as a follow-up issue. After sessionId resets, hasInitializedRef stays false until the first non-empty messages pass — meaning the first artifact arrival is treated as hydration baseline, not as a live event to auto-open.
There was a problem hiding this comment.
📋 Automated Review — PR #12629
PR #12629 — feat(platform): add copilot artifact preview panel
Author: ntindle | Files: 45
🎯 Verdict: REQUEST_CHANGES
PR Description Quality
✅ Has Why + What + How — Thorough description with Changes section listing 8 categories, fully checked checklist, and a "post-review hardening" section documenting addressed feedback.
What This PR Does
Adds a dedicated artifact preview panel to the copilot chat interface. When the AI generates files (HTML, CSV, PDF, code, React/TSX), users can preview them inline with type-specific renderers, navigate artifact history, resize/minimize/maximize the panel, and download or copy content. The backend gains a new list_workspace_files endpoint with pagination and metadata origin tracking (user-upload vs agent-created). The entire feature is gated behind a Flag.ARTIFACTS feature flag for safe rollout.
Specialist Findings
🛡️ Security ✅ — Well-designed sandbox model. HTML/React iframes use sandbox="allow-scripts" without allow-same-origin, giving them opaque origins with no access to parent cookies/storage/DOM. React CDN scripts have SRI hashes. </script> breakout is properly escaped. The deliberate omission of CSP is documented in iframe-sandbox-csp.ts with clear rationale.
- 🟡 PDF iframe at
ArtifactContent.tsx:98has no sandbox attribute (Chrome limitation documented); blob URL null origin limits risk but worth monitoring. - 🔵 Backend
routes.py:280returnsValueErrormessage directly in HTTP response, potentially leaking internal paths. Low risk behind auth.
🏗️ Architecture ✅ — Clean separation: classification (pure functions) → content fetching (hooks) → rendering (components) → state (Zustand store). Feature-flagged with next/dynamic lazy loading. Follows ComponentName/useComponentName/helpers pattern throughout.
- 🟡
store.tsCopilotUIStatehas grown to 20+ fields spanning notifications, sound, drawer, sessions, and artifact panel — approaching the point where splitting into slices would improve maintainability. - 🟡
reactArtifactPreview.tsembeds a 270-line JS runtime as a template literal that isn't linted or type-checked. Consider extracting to a standalone asset. (Flagged by: architect, quality — 2) - 🔵 Module-level
contentCacheinuseArtifactContent.ts:14persists across session switches;clearContentCache()only runs on explicit data reset.
⚡ Performance ✅ — Acceptable for typical copilot usage. Good patterns: LRU content cache, debounced width persistence, contentVisibility: auto on CSV tables, scroll position preservation.
- 🟠 Regex compilation inside loop at
helpers.ts:271— twonew RegExp(...)compiled per iteration inextractWorkspaceArtifacts. O(n) regex compilations for messages with many workspace URIs. (Flagged by: performance, prior review — 2) - 🟡 CSV rendering at
CSVRenderer.tsx:113creates all DOM nodes at once — no row virtualization for large datasets. - 🔵
useAutoOpenArtifacts.ts:75createsnew Set(messages.map(m => m.id))on every streaming chunk.
🧪 Testing
- 🟠
useArtifactPanel.ts:89—handleCopyhas three code paths (cached content, fetch fallback, clipboard error) with zero test coverage. (Flagged by: testing, prior review — 2) - 🟠
ArtifactContent.tsx:80— Renderer routing logic (html→iframe, code→codeRenderer, csv→csvRenderer, json→jsonRenderer, image→img, pdf→iframe) has no test coverage. A misroute would be invisible to the current suite. - 🟠
routes.py:371—list_workspace_filesempty stringsession_idnormalization (session_id = session_id or None) is untested. - 🟡
CSVRenderer.test.ts:46— Render tests only assertnot.toThrow()without verifying parsed output correctness.
📖 Quality ✅ — Consistently styled, follows team conventions (Phosphor icons only, no any types, no barrel files, interface Props pattern). Well-documented where complexity warrants it (CSP rationale, security model).
- 🟠
ChatMessagesContainer/helpers.tsat 360 lines exceeds the ~200 line guideline and mixes four concerns (render segments, marker parsing, artifact extraction, URL resolution). (Flagged by: quality, architect — 2) - 🔵
reactArtifactPreview.ts:55—runtimeis a vague variable name for the escaped compiled code JSON. - 🔵
store.ts:205—MAX_HISTORY = 25defined inside action closure; should be module-level constant.
📦 Product ✅ — All claimed features delivered. Error states show retry, loading uses skeleton, mobile gets fullscreen Sheet overlay. Keyboard accessible (Escape closes, drag handle has proper ARIA).
- 🟡 Auto-open at
useAutoOpenArtifacts.ts:70fires per-artifact during streaming, which can interrupt users viewing a previous artifact. - 🟡 Files over 10MB silently become download-only with no user-facing explanation (
helpers.ts:203). - 🔵
SourceToggle.tsx:11— Preview/Source toggle buttons lackrole="group"wrapper for screen readers.
📬 Discussion
- 🟠
routes.py:278— 413 vs 409 error mapping relies on string prefix matching (message.startswith("File too large")), which is fragile if the error message wording changes. A custom exception subclass would be more robust. - 🟡
useAutoOpenArtifactsruns unconditionally even whenFlag.ARTIFACTSis off (ChatContainer.tsx:12).
🔎 QA ✅ — All 22 backend API scenarios pass (list, upload, download, delete, pagination, auth, 409/413 errors, session scoping, empty session normalization). All 110 tests pass (88 frontend + 22 backend). Artifact panel UI is behind feature flag and cannot be live-tested without LaunchDarkly.
🟠 Should Fix
- Missing test:
useArtifactPanelcopy logic (useArtifactPanel.ts:89) — Three untested code paths (cache hit, fetch fallback, clipboard error). Add tests for each. (Flagged by: testing, prior review — 2) - Missing test:
ArtifactContentrenderer routing (ArtifactContent.tsx:80) — No test verifies the classification-to-renderer mapping. A bug in the if/else chain would pass all existing tests. Add at least one test per renderer type. (Flagged by: testing — 1) - Missing test: empty
session_idnormalization (routes.py:371) —session_id = session_id or Noneis the real-world regression vector but has no test. Addtest_list_files_empty_session_id. (Flagged by: testing — 1) - Regex compiled inside loop (
helpers.ts:271) — Twonew RegExp(...)per loop iteration inextractWorkspaceArtifacts. Hoist outside the loop. (Flagged by: performance, prior review — 2) helpers.tsexceeds file-length guideline (ChatMessagesContainer/helpers.ts, 360 lines) — Mixes four distinct concerns. Split intorenderSegments.ts,markers.ts,artifacts.ts,workspaceUrls.ts. (Flagged by: quality, architect — 2)- Fragile error string matching (
routes.py:278) —message.startswith("File too large")to choose 413 vs 409 will break silently if the error message changes. Use a custom exception subclass. (Flagged by: discussion — 1)
🟡 Nice to Have
- Extract React preview runtime (
reactArtifactPreview.ts:49) — 270-line inline JS template isn't linted or type-checked. Extracting to a standalone.jsasset would enable tooling. (architect, quality) - Row virtualization for CSV (
CSVRenderer.tsx:113) — All rows rendered to DOM at once; large CSVs will cause DOM bloat.contentVisibility: autohelps but isn't sufficient for 10k+ rows. (performance) - Pre-warm TypeScript compiler (
transpileReactArtifact.ts:12) — First React preview incurs ~1-3s delay loading the 5MB TS compiler. Trigger import when panel opens with React classification. (performance) - Debounce auto-open during streaming (
useAutoOpenArtifacts.ts:70) — Panel switches focus on every new artifact during a long streaming response, interrupting users. (product) - 10MB download-only tooltip (
helpers.ts:203) — Large files silently lose preview with no user explanation. Add tooltip. (product) useAutoOpenArtifactsruns when flag is off (ChatContainer.tsx:12) — Wastes cycles fingerprinting artifacts when the panel can't render. Gate behindFlag.ARTIFACTS. (discussion)
🔵 Nits
- Vague variable name (
reactArtifactPreview.ts:55) —runtimeshould besafeCompiledCodeorescapedCodeJson. - Inline constant (
store.ts:205) —MAX_HISTORY = 25should be module-level. - Missing
role="group"(SourceToggle.tsx:11) — Wrap toggle buttons for screen reader grouping.
QA Screenshots
| Screenshot | Description |
|---|---|
 |
Copilot page loads correctly; artifact panel hidden behind feature flag ✅ |
 |
Final state after QA — no errors in UI ✅ |
Human Review Needed
YES — 45-file feature PR with security-sensitive iframe sandboxing, new Function() eval-equivalent execution of AI-generated code, and backend API surface expansion. The sandbox model is well-designed but warrants human verification of the threat model assumptions.
Risk Assessment
Merge risk: LOW | Rollback: EASY — Feature-flagged behind Flag.ARTIFACTS (defaults to false). Disabling the flag completely hides the panel. Backend changes are additive (new endpoint, new metadata field) and don't modify existing behavior.
CI Status
❌ 5/6 checks failed — Frontend lint, typecheck, build, and both test suites reported failures (appear to be environment/timeout issues rather than code issues, as all tests pass when run directly). Backend lint passes.
Issues attributed to commits in this pull requestThis pull request was merged and Sentry observed the following issues:
|
5 participants
Why / What / How
Copilot artifacts were not previewing reliably: PDFs downloaded instead of rendering, Python code could still render like markdown, JSX/TSX artifacts were brittle, HTML dashboards/charts could fail to execute, and users had to manually open artifact panes after generation. The pane also got stuck at maximized width when trying to drag it smaller.
This PR adds a dedicated copilot artifact panel and preview pipeline across the backend/frontend boundary. It preserves artifact metadata needed for classification, adds extension-first preview routing, introduces dedicated preview/rendering paths for HTML/CSV/code/PDF/React artifacts, auto-opens new or edited assistant artifacts, and fixes the maximized-pane resize path so dragging exits maximized mode immediately.
Changes 🏗️
add artifact card and artifact panel UI in copilot, including persisted panel state and resize/maximize/minimize behavior
add shared artifact extraction/classification helpers and auto-open behavior for new or edited assistant messages with artifacts
add preview/rendering support for HTML, CSV, PDF, code, and React artifact files
fix code artifacts such as Python to render through the code renderer with a dark code surface instead of markdown-style output
improve JSX/TSX preview behavior with provider wrapping, fallback export selection, and explicit runtime error surfaces
allow script execution inside HTML previews so embedded chart dashboards can render
update workspace artifact/backend API handling and regenerate the frontend OpenAPI client
add regression coverage for artifact helpers, React preview runtime, auto-open behavior, code rendering, and panel store behavior
post-review hardening: correct download path for cross-origin URLs, defer scroll restore until content mounts, gate auto-open behind the ARTIFACTS flag, parse CSVs with RFC 4180-compliant quoted newlines + BOM handling, distinguish 413 vs 409 on upload, normalize empty session_id, and keep AnimatePresence mounted so the panel exit animation plays
Checklist 📋
For code changes:
pnpm formatpnpm lintpnpm typespnpm test:unitFor configuration changes:
.env.defaultis updated or already compatible with my changesdocker-compose.ymlis updated or already compatible with my changesNote
Medium Risk
Adds a new Copilot artifact preview surface that executes user/AI-generated HTML/React in sandboxed iframes and changes workspace file upload/listing behavior, so regressions could affect file handling and client security assumptions despite sandboxing safeguards.
Overview
Adds an Artifacts feature (flagged by
Flag.ARTIFACTS) to Copilot: workspace file links/attachments now render asArtifactCards and can open a new resizable/minimizableArtifactPanelwith history, auto-open behavior, copy/download actions, and persisted panel width.Introduces a richer artifact preview pipeline with type classification and dedicated renderers for HTML, CSV, PDF, code (Shiki-highlighted), and React/TSX (transpiled and executed in a sandboxed iframe), plus safer download filename handling and content caching/scroll restore.
Extends the workspace backend API by adding
GET /workspace/filespagination, standardizing operation IDs in OpenAPI, attachingmetadata.originon uploads/agent-created files, normalizing emptysession_id, improving upload error mapping (409 vs 413), and hardening post-quota soft-delete error handling; updates and expands test coverage accordingly.Reviewed by Cursor Bugbot for commit b732d10. Bugbot is set up for automated code reviews on this repo. Configure here.