Feat: Playground - Need to accept File uploading#12326
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
Frontend Unit Test Coverage ReportCoverage Summary
Unit Test Results
|
Codecov Report❌ Patch coverage is ❌ Your project status has failed because the head coverage (49.57%) is below the target coverage (60.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## release-1.9.0 #12326 +/- ##
=================================================
+ Coverage 52.45% 52.63% +0.17%
=================================================
Files 2013 2016 +3
Lines 182350 182517 +167
Branches 26912 27056 +144
=================================================
+ Hits 95659 96072 +413
+ Misses 85616 85369 -247
- Partials 1075 1076 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Empreiteiro
left a comment
Empreiteiro
left a comment
There was a problem hiding this comment.
Successful Tests:
- Uploading a text file directly to the playground.
- Uploading a text file to the chat input.
- Uploading a PDF that has an image instead of text: Doesn't read, but I consider it OK. For OCR, I suggest using the File Reader component.
- Uploading an image via the playground.
Unsuccessful Tests:
- Uploading an image via the chat input: Breaks the instance, but after returning, the response is correct.
UI/UX Change Suggestions:
- Change the file upload icon in the playground (the current icon refers to an image)
- After adding a file to the chat input, it is not possible to remove it. This greatly degrades the building experience, as it is necessary to delete the component and add another.
|
This video demonstrates the error when uploading an image file in Chat Input: Gravacao.de.Tela.2026-04-01.115651.mp4 |
Cristhianzl
left a comment
Cristhianzl
left a comment
There was a problem hiding this comment.
Summary
This PR expands the playground and chat input to accept non-image file uploads (PDF, CSV, JSON, DOCX, code files, etc.). It refactors duplicated file upload logic into a shared hook, adds file validation utilities, updates the backend to convert non-image attachments to text, and adds test coverage.
📊 Overall Score
| Category | Status | Details |
|---|---|---|
| 🔴 CRITICAL: Security & PII | Silent error swallowing in backend | |
| 🔴 CRITICAL: DRY | Duplicated test file, duplicated normalizeFilePath logic |
|
| 🔴 CRITICAL: File Structure | ✅ Pass | All files within limits |
| 🟠 IMPORTANT: SOLID | SRP concerns, YAGNI in path normalization | |
| 🟠 IMPORTANT: Error Handling | Silent failures, broad except |
|
| 🟠 IMPORTANT: Code Quality | any types, magic values, naming |
|
| 🟡 RECOMMENDED: Comments | ✅ Pass | |
| 🟢 TESTING | Duplicate tests, missing adversarial cases |
🔴 CRITICAL Findings
C1. Silent Error Swallowing in Backend (Security / Error Handling)
File: src/lfx/src/lfx/schema/message.py lines 253-289
except Exception as exc: # noqa: BLE001
logger.warning(f"Skipping unsupported attachment during message conversion: {type(exc).__name__}")What the assumption is: Any exception during file processing is non-critical and can be silently skipped.
Why it's wrong: This catches ALL exceptions, including:
PermissionError— could indicate a security issue (the process is accessing files it shouldn't)MemoryError— processing a huge file could crash the serverKeyboardInterrupt(caught by bareExceptionin some contexts)- Bugs in
parse_text_file_to_data()— would be silently masked
What the blast radius is: A bug in the file parsing pipeline would be invisible. Users would get no feedback that their file wasn't processed. Debugging production issues would be extremely difficult.
What the fix is:
except (FileNotFoundError, ValueError, UnicodeDecodeError, OSError) as exc:
logger.warning(
"Skipping unsupported attachment during message conversion",
extra={"error_type": type(exc).__name__, "file_name": Path(file).name}
)Additionally, the inner except Exception on line 267 for Path(file).stat() has the same issue:
try:
file_size_bytes = Path(file).stat().st_size
except Exception: # noqa: BLE001
file_size_bytes = NoneIf stat() fails, the file is allowed through without size checking. This should catch specific exceptions:
except (OSError, ValueError):
file_size_bytes = NoneSeverity: 🔴 BLOCKER — Silent failures violate error handling rules (no silent failures, no generic exceptions).
C2. Duplicated Test File — DRY Violation
Files:
src/frontend/src/shared/hooks/__tests__/use-chat-file-upload.test.ts(67 lines)src/frontend/src/utils/__tests__/fileUtils.test.ts(90 lines)
Problem: The test file use-chat-file-upload.test.ts does NOT test the useChatFileUpload hook at all. It tests isAllowedChatAttachmentFile — the exact same function already tested in fileUtils.test.ts. The test cases overlap significantly:
| Test case | use-chat-file-upload.test.ts |
fileUtils.test.ts |
|---|---|---|
| allows png | ✅ | ✅ |
| allows jpg | ✅ | (bmp instead) |
| allows pdf | ✅ | (csv instead) |
| allows csv | ✅ | ✅ |
| allows docx | ✅ | — |
| blocks spoofing | ✅ | ✅ |
| no extension + allowed mime | ✅ | ✅ |
| extension only (no mime) | ✅ | ✅ |
| blocks unsupported | ✅ | ✅ |
The file is named use-chat-file-upload.test.ts but imports from @/utils/fileUtils — misleading name, wrong location, duplicate content.
Fix: Delete src/frontend/src/shared/hooks/__tests__/use-chat-file-upload.test.ts entirely. If hook-level tests are needed, they should test the hook's behavior (calling mutate, setting files state, error handling), not re-test the validation utility.
Severity: 🔴 BLOCKER — DRY violation (duplicate test logic) + misleading file name.
C3. Duplicated normalizeFilePath Logic — DRY / YAGNI
Files:
src/frontend/src/components/core/playgroundComponent/chat-view/utils/file-utils.ts→ newnormalizeServerImagePath()- The same module already has path normalization in
getFilePreviewUrl()(pre-existing)
Problem: The new normalizeServerImagePath() function (37 lines) duplicates and extends the path normalization that already existed inline in getFilePreviewUrl(). The old code did:
const path = file.trim().replace(/\\/g, "/");The new function adds:
isAbsoluteUrl()checklangflowsegment detection (extract path after "langflow" in path)- UUID segment detection (regex-based)
- Leading slash stripping
Questions 1 & 5 from Security Mindset:
- What is this code trusting? It trusts that any path containing "langflow" or a UUID pattern should be truncated. A filename like
langflow-report/data.csvwould be incorrectly normalized. - What is the blast radius? Broken file URLs for any file whose path happens to contain "langflow" as a segment or a UUID-like pattern.
The langflow segment detection is particularly fragile:
const langflowIdx = lowerParts.lastIndexOf("langflow");
if (langflowIdx !== -1 && langflowIdx + 1 < parts.length) {
return parts.slice(langflowIdx + 1).join("/");
}This will match ANY path containing a segment named "langflow", not just the cache directory.
Fix: This function is YAGNI for the stated PR goal ("accept non-image file uploads"). Path normalization for macOS cache paths and Windows paths is a separate concern. If needed, it should be a separate PR with proper edge case handling.
Severity: 🔴 BLOCKER — YAGNI (solves a problem not in the current task requirements) + fragile heuristic.
🟠 IMPORTANT Findings
I1. Missing Return Types — Weak Typing
File: src/frontend/src/utils/fileUtils.ts
export const getFileExtension = (fileName: string) => { // ❌ no return type
export const hasFileExtension = (fileName: string) => { // ❌ no return typeFix:
export const getFileExtension = (fileName: string): string => {
export const hasFileExtension = (fileName: string): boolean => {Severity: 🟠 IMPORTANT — Strong typing rule violation.
I2. any Types in Backend Tests
File: src/backend/tests/unit/test_messages.py
Not a TypeScript issue, but the test test_to_lc_message_skips_oversized_file_attachments creates a 1GB+ sparse file:
with open(big_path, "wb") as handle:
handle.truncate(1024 * 1024 * 1024 + 1)This creates a 1GB sparse file on disk. While sparse files don't consume actual disk space on most filesystems, this is:
- Platform-dependent behavior (Windows NTFS handles sparse files differently)
- Potentially slow in CI environments
- The 1GB+1 byte value is a magic number — should reference
_MAX_ATTACHMENT_SIZE_BYTES
Fix:
from lfx.schema.message import Message
big_size = Message._MAX_ATTACHMENT_SIZE_BYTES + 1
with open(big_path, "wb") as handle:
handle.truncate(big_size)Severity: 🟠 IMPORTANT — Magic value + potential CI issue.
I3. any Types in Frontend
File: src/frontend/src/shared/hooks/use-chat-file-upload.ts line 81
onError: (error) => {
// ...
error.response?.data?.detailThe error parameter has no type annotation. It's implicitly any, relying on the shape error.response?.data?.detail which is Axios-specific but not typed.
Fix: Type the error explicitly:
import type { AxiosError } from "axios";
onError: (error: AxiosError<{ detail?: string }>) => {Severity: 🟠 IMPORTANT — any type rule violation.
I4. Hardcoded 1GB Limit — Magic Value
File: src/lfx/src/lfx/schema/message.py line 79
_MAX_ATTACHMENT_SIZE_BYTES = 1024 * 1024 * 1024This is a class-level attribute with no configuration mechanism. 1GB is an extremely high limit for an attachment that will be converted to text and sent to an LLM. Consider:
- LLM context windows are typically 128K-1M tokens
- A 1GB text file would produce billions of tokens
- The
parse_text_file_to_data()function would need to read the entire file into memory
Fix: Either:
- Reduce the limit to something practical (e.g., 50MB)
- Make it configurable via settings/environment variable
- Add a comment explaining why 1GB was chosen
Severity: 🟠 IMPORTANT — Magic value + potential resource exhaustion.
I5. Feature Flag Not Propagated to Shared Hook
File: src/frontend/src/modals/IOModal/components/chatView/chatInput/chat-input.tsx lines 56-63
const handleFileChange = async (event) => {
if (playgroundPage && !ENABLE_IMAGE_ON_PLAYGROUND) {
if ("target" in event && event.target instanceof HTMLInputElement) {
event.target.value = "";
}
return;
}
handleFileUploadChange(event);
};The ENABLE_IMAGE_ON_PLAYGROUND feature flag check is done OUTSIDE the shared hook, creating a leaky abstraction. The playground chat-input.tsx does NOT have this check at all (it was removed in the refactoring).
Question: Is this intentional? After the refactoring, the playground chat-input no longer checks ENABLE_IMAGE_ON_PLAYGROUND. If this flag is false, the playground will still allow file uploads (the old code prevented this).
Fix: Either:
- Add the feature flag check inside
useChatFileUpload(passplaygroundPageas param) - Or document that this is intentional behavior change
Severity: 🟠 IMPORTANT — Potential regression in feature flag behavior.
I6. playgroundPage Added to ChatInputType But Not Optional
File: src/frontend/src/types/components/index.ts line 566
export type ChatInputType = {
// ...
playgroundPage: boolean; // NEW — not optional
};This is a non-optional field added to an existing type. Every consumer of ChatInputType must now provide this prop. Was every consumer updated? If any caller doesn't provide it, TypeScript will catch it at build time, but this could cause build failures in downstream consumers (like Langflow Desktop).
Severity: 🟠 IMPORTANT — Breaking change to shared type.
🟡 RECOMMENDED Findings
R1. File Name Violates Convention: fileUtils.ts
File: src/frontend/src/utils/fileUtils.ts
The REVIEWER_RULE.md states: "No generic file names: utils, helpers, misc, common, shared (standalone)"
fileUtils.ts is a generic utils file. It contains:
getFileExtension()— parsinghasFileExtension()— validationisAllowedChatAttachmentFile()— validation
All three are validation/parsing functions. Per the file structure rules, these should be in a file named something like file-validation.ts or chat-attachment-validation.ts.
Severity: 🟡 RECOMMENDED — Naming convention.
R2. Inconsistent Naming Between Constants
File: src/frontend/src/constants/constants.ts
The PR adds multiple constant groups with inconsistent naming patterns:
// Image-specific (NEW)
ALLOWED_IMAGE_INPUT_EXTENSIONS // array
ALLOWED_IMAGE_INPUT_MIME_TYPES // array
CHAT_IMAGE_UPLOAD_ACCEPT // string
CHAT_IMAGE_UPLOAD_TOOLTIP // string
// Attachment (NEW)
ALLOWED_CHAT_ATTACHMENT_INPUT_EXTENSIONS // array
ALLOWED_CHAT_ATTACHMENT_INPUT_MIME_TYPES // array
CHAT_ATTACHMENT_UPLOAD_ACCEPT // string
CHAT_ATTACHMENT_UPLOAD_TOOLTIP // stringThe naming is inconsistent:
ALLOWED_IMAGE_INPUT_*vsALLOWED_CHAT_ATTACHMENT_INPUT_*— different prefix patternsCHAT_IMAGE_UPLOAD_*vsCHAT_ATTACHMENT_UPLOAD_*— different prefix patterns- The word order varies:
IMAGE_INPUTvsCHAT_ATTACHMENT_INPUT
Fix: Standardize naming, e.g.:
CHAT_UPLOAD_IMAGE_EXTENSIONS
CHAT_UPLOAD_IMAGE_MIME_TYPES
CHAT_UPLOAD_ATTACHMENT_EXTENSIONS
CHAT_UPLOAD_ATTACHMENT_MIME_TYPESSeverity: 🟡 RECOMMENDED — Naming consistency.
R3. Constants File Growing Without Bound
File: src/frontend/src/constants/constants.ts
This PR adds ~70 lines to an already large constants file. The file-upload-specific constants (extensions, MIME types, accept strings, tooltips) are a distinct concern and could live in their own constants file (e.g., file-upload-constants.ts).
Severity: 🟡 RECOMMENDED — File cohesion.
R4. Unrelated Formatting Changes Pollute the Diff
Files:
src/frontend/src/utils/reactflowUtils.ts— 27 lines of pure indentation changessrc/frontend/src/utils/__tests__/cleanEdges.test.ts— 13 lines of pure indentation changes
These are autofix.ci changes that have nothing to do with the feature. They make the PR harder to review.
Fix: These should be in a separate autoformat commit or excluded from the PR.
Severity: 🟡 RECOMMENDED — PR hygiene.
🟢 TESTING Findings
T1. Missing Adversarial Tests for useChatFileUpload Hook
File: src/frontend/src/shared/hooks/__tests__/use-chat-file-upload.test.ts
As noted in C2, this file doesn't test the hook at all. The hook has complex behavior that is untested:
- What happens when
mutatefails? - What happens when
validateFileSizethrows? - What happens with clipboard paste events?
- What happens when
currentFlowIdis empty? - What happens when
setFilescallback is called with stale state?
Fix: Write actual hook tests using @testing-library/react-hooks that test:
- Happy path: file upload succeeds, files state updated
- Error path: file upload fails, error state set
- Validation path: invalid file rejected before upload
- Size path: oversized file rejected
- Clipboard path: paste event handled correctly
Severity: 🟢 TESTING — Missing adversarial tests for new hook.
T2. Backend Tests Don't Verify Log Output
File: src/backend/tests/unit/test_messages.py
The test test_to_lc_message_skips_unsupported_file_attachments verifies the file is skipped, but doesn't verify the warning log is emitted. Since the warning is the only signal that something went wrong, it should be verified:
def test_to_lc_message_skips_unsupported_file_attachments(caplog):
# ... existing setup ...
with caplog.at_level(logging.WARNING):
lc_message = message.to_lc_message()
assert "Skipping unsupported attachment" in caplog.textSeverity: 🟢 TESTING — Incomplete assertion.
T3. Missing Edge Case: normalizeServerImagePath with Misleading Segments
File: src/frontend/src/components/core/playgroundComponent/chat-view/utils/__tests__/file-utils.test.ts
No test covers the fragile langflow segment detection edge case:
// This would be incorrectly normalized:
normalizeServerImagePath("/uploads/langflow-report/data.csv")
// Expected: "langflow-report/data.csv" (or keep as-is)
// Actual: "data.csv" (truncated because "langflow" not found as exact segment... actually OK)
// But what about:
normalizeServerImagePath("/uploads/langflow/my-project/flow123/data.csv")
// Returns: "my-project/flow123/data.csv" — is this correct?Severity: 🟢 TESTING — Missing edge case coverage for path normalization heuristic.
🔴 Security Review Mindset — The Five Questions
| # | Question | Finding |
|---|---|---|
| 1 | What is this code trusting without verifying? | The backend trusts parse_text_file_to_data() to handle any file safely. No content-type verification at the backend level — the frontend checks extension+MIME but the backend just tries to parse anything. |
| 2 | What is the authoritative source for this behavior? | The file type lists appear reasonable but text/mdx and application/mdx are non-standard MIME types not registered with IANA. The application/x-yaml and text/yaml are both included (correct — both are used in practice). |
| 3 | What happens in every failure path? | |
| 4 | Who controls each value, and could they lie? | MIME types come from the browser (File.type) which can be spoofed. The dual validation (extension + MIME) in isAllowedChatAttachmentFile() is good practice and catches simple spoofing. However, a .pdf file with application/pdf MIME but containing malicious content would pass validation — the backend relies on parse_text_file_to_data() to handle this safely. |
| 5 | What is the blast radius if this is wrong? | Low-medium. If file parsing fails silently, users get no response for their file (bad UX, not a security issue). If a malicious file passes validation and crashes parse_text_file_to_data(), the broad except catches it silently. No direct DB access or code execution risk from the file content. |
Empreiteiro
left a comment
Empreiteiro
left a comment
There was a problem hiding this comment.
I performed all the tests below and they all worked:
- Uploading a text file directly to the playground.
- Uploading a text file to the chat input.
- Uploading an image via the playground.
- Uploading an image via the chat input.
- Testing with different models and providers.
|
test is failing consistently |
* added file upload to play ground * coderabbit suggestions * increase file size to 1gb * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * Improve server image support * code rabbit fix * [autofix.ci] apply automated fixes * critical bug * fix recommended testcases * Feature Flag propergated * add adversal testcases * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * rename files and improve file structure * [autofix.ci] apply automated fixes * avoid Pydantic private attrs for attachment max size * [autofix.ci] apply automated fixes * ruff update * fix invalid image path * playground icon-Coins * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * chat-view santization to improve typing reverted * add translation * [autofix.ci] apply automated fixes * loosen testcase --------- Co-authored-by: Olayinka Adelakun <olayinkaadelakun@Olayinkas-MacBook-Pro.local> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Olayinka Adelakun <olayinkaadelakun@mac.war.can.ibm.com> Co-authored-by: Carlos Coelho <80289056+carlosrcoelho@users.noreply.github.com>
6 participants
Description
The goal of this change is to add support for non-image file uploads to both the playground and the chatInput component. I ensure it supports a wide range of file input types (I am still confirming the file types and sizes we should accept).
Testcase #1
Testcase #2
Screenshot
Before
https://github.com/user-attachments/assets/f298a606-8cab-4271-83d2-f1a6443c8ecb
After
