Fix CSV preview duplicate key warning (#10920)#17754
Fix CSV preview duplicate key warning (#10920)#17754bugisthegod wants to merge 1 commit intotwentyhq:mainfrom
Conversation
Welcome!
Hello there, congrats on your first PR! We're excited to have you contributing to this project. |
Greptile OverviewGreptile SummaryThis PR replaces the DocViewer CSV rendering path with a custom CSV table to avoid React duplicate key warnings caused by Main issues to address before merge:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant U as User
participant DV as DocumentViewer
participant F as fetchCsvPreview
participant Net as fetch(documentUrl)
participant P as Papa.parse
U->>DV: Open file preview (documentUrl, documentName)
alt fileExtension == csv
DV->>F: fetchCsvPreview(documentUrl)
F->>Net: GET documentUrl
Net-->>F: Response
F->>F: response.text()
F->>P: parse(text, { preview: rows+1, header:false })
P-->>F: { data, errors }
F-->>DV: { headers, rows }
DV-->>U: Render <table> headers/rows
else other previewable
DV-->>U: Render DocViewer
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| useEffect(() => { | ||
| if (fileExtension === 'csv') { | ||
| fetchCsvPreview(documentUrl).then((content) => { | ||
| setCsvPreview(content); | ||
| }); | ||
| fetchCsvPreview(documentUrl).then(setCsvPreview); | ||
| } | ||
| }, [documentUrl, fileExtension]); |
There was a problem hiding this comment.
CSV preview stuck loading
fetchCsvPreview(documentUrl).then(setCsvPreview) has no cancellation or error handling. If the fetch/parse rejects (network/CORS/non-2xx causing response.text() to throw, etc.), the component will stay on “Loading csv ...” and you’ll get an unhandled rejection. It can also race when documentUrl changes quickly, setting CSV state for the previous URL after the new one. Consider aborting/guarding the effect and handling errors by rendering the existing “Preview Not Available / Download” UI for CSV.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/twenty-front/src/modules/activities/files/components/DocumentViewer.tsx
Line: 197:201
Comment:
**CSV preview stuck loading**
`fetchCsvPreview(documentUrl).then(setCsvPreview)` has no cancellation or error handling. If the fetch/parse rejects (network/CORS/non-2xx causing `response.text()` to throw, etc.), the component will stay on “Loading csv ...” and you’ll get an unhandled rejection. It can also race when `documentUrl` changes quickly, setting CSV state for the previous URL after the new one. Consider aborting/guarding the effect and handling errors by rendering the existing “Preview Not Available / Download” UI for CSV.
How can I resolve this? If you propose a fix, please make it concise.| export const fetchCsvPreview = async (url: string): Promise<CsvPreviewData> => { | ||
| const response = await fetch(url); | ||
| const text = await response.text(); |
There was a problem hiding this comment.
Non-OK responses parsed
fetchCsvPreview parses await response.text() without checking response.ok, so a 403/404 HTML body (or any non-CSV content) will still be parsed and rendered as a “CSV” table. PapaParse also exposes result.errors which is currently ignored, so malformed CSV can silently produce surprising headers/rows. This should be treated as a fetch/parse failure so the caller can show the normal “preview unavailable” path instead of rendering garbage.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/twenty-front/src/modules/activities/files/utils/fetchCsvPreview.ts
Line: 10:12
Comment:
**Non-OK responses parsed**
`fetchCsvPreview` parses `await response.text()` without checking `response.ok`, so a 403/404 HTML body (or any non-CSV content) will still be parsed and rendered as a “CSV” table. PapaParse also exposes `result.errors` which is currently ignored, so malformed CSV can silently produce surprising `headers/rows`. This should be treated as a fetch/parse failure so the caller can show the normal “preview unavailable” path instead of rendering garbage.
How can I resolve this? If you propose a fix, please make it concise.![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| fetchCsvPreview(documentUrl).then((content) => { | ||
| setCsvPreview(content); | ||
| }); | ||
| fetchCsvPreview(documentUrl).then(setCsvPreview); |
There was a problem hiding this comment.
Bug: The promise returned by fetchCsvPreview lacks a .catch() handler. A failed network request will cause the component to show a loading state indefinitely.
Severity: MEDIUM
Suggested Fix
Add a .catch() block to the promise chain after the call to fetchCsvPreview. Inside the handler, log the error and update the component's state to display an error message to the user instead of the loading indicator.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location:
packages/twenty-front/src/modules/activities/files/components/DocumentViewer.tsx#L199
Potential issue: The `useEffect` hook that calls `fetchCsvPreview` for CSV files does
not handle promise rejections. If the underlying `fetch` call fails due to a network
error, CORS issue, or an invalid URL, the promise will reject. Because there is no
`.catch()` block, the `csvPreview` state will never be updated, and the component will
remain indefinitely in a "Loading csv ..." state, with no error message or recovery path
for the user.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
Pull request overview
This PR fixes console warnings about duplicate React keys when previewing CSV files by implementing a custom CSV table renderer instead of relying on the @cyntler/react-doc-viewer library's flawed CSV renderer that used cell values as keys.
Changes:
- Refactored
fetchCsvPreviewto return structured data (headers and rows arrays) instead of reformatted CSV text - Implemented custom CSV table component in
DocumentViewerwith proper React keys using indices - Removed dependency on DocViewer library for CSV rendering
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
packages/twenty-front/src/modules/activities/files/utils/fetchCsvPreview.ts |
Changed return type from string to structured CsvPreviewData object with headers and rows arrays; modified CSV parsing to not use header mode |
packages/twenty-front/src/modules/activities/files/components/DocumentViewer.tsx |
Added custom StyledCsvTable component and rendering logic for CSV files; replaced DocViewer blob URL approach with direct table rendering |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export const fetchCsvPreview = async (url: string): Promise<CsvPreviewData> => { | ||
| const response = await fetch(url); | ||
| const text = await response.text(); | ||
|
|
||
| const result = Papa.parse(text, { | ||
| preview: DEFAULT_PREVIEW_ROWS, | ||
| const result = Papa.parse<string[]>(text, { | ||
| preview: DEFAULT_PREVIEW_ROWS + 1, // +1 for header row | ||
| skipEmptyLines: true, | ||
| header: true, | ||
| header: false, | ||
| }); | ||
|
|
||
| const data = result.data as Record<string, string>[]; | ||
|
|
||
| const csvContent = Papa.unparse(data, { | ||
| header: true, | ||
| }); | ||
| const [headers = [], ...rows] = result.data; | ||
|
|
||
| return csvContent; | ||
| return { headers, rows }; | ||
| }; |
There was a problem hiding this comment.
The fetchCsvPreview function lacks test coverage. Other utility functions in this directory (downloadFile, getFileType) have corresponding test files in tests/. Consider adding tests for this function to verify correct CSV parsing, header extraction, and edge cases like empty CSVs, CSVs with only headers, or malformed data.
| }; | ||
|
|
||
| export const fetchCsvPreview = async (url: string): Promise<CsvPreviewData> => { | ||
| const response = await fetch(url); |
There was a problem hiding this comment.
The fetch call lacks error handling for network failures or non-2xx HTTP status codes. If the fetch fails or returns an error status, the function will throw an unhandled error. Consider adding error handling similar to other fetch calls in the codebase, which check response.ok and throw appropriate errors with status information.
| const response = await fetch(url); | |
| let response: Response; | |
| try { | |
| response = await fetch(url); | |
| } catch (error) { | |
| const message = error instanceof Error ? error.message : String(error); | |
| throw new Error(`Failed to fetch CSV preview from "${url}": ${message}`); | |
| } | |
| if (!response.ok) { | |
| throw new Error( | |
| `Failed to fetch CSV preview from "${url}": ${response.status} ${response.statusText}`, | |
| ); | |
| } |
| const result = Papa.parse<string[]>(text, { | ||
| preview: DEFAULT_PREVIEW_ROWS + 1, // +1 for header row | ||
| skipEmptyLines: true, | ||
| header: true, | ||
| header: false, | ||
| }); |
There was a problem hiding this comment.
Papa.parse can encounter errors during CSV parsing (e.g., malformed CSV files). The result object has an 'errors' array that should be checked. Consider validating result.errors and handling parse errors gracefully to prevent displaying malformed data or providing unclear error messages to users.
| const csvContent = Papa.unparse(data, { | ||
| header: true, | ||
| }); | ||
| const [headers = [], ...rows] = result.data; |
There was a problem hiding this comment.
If the CSV file is empty or contains only a header row with no data rows, the destructuring will work but rows will be an empty array. While this is technically correct, consider whether an empty CSV should show an empty table or a specific message to users indicating there's no data to preview.
| const StyledCsvTable = styled.table` | ||
| width: 100%; | ||
| text-align: left; | ||
| border-collapse: collapse; | ||
| color: ${({ theme }) => theme.font.color.primary}; | ||
| th, | ||
| td { | ||
| padding: 5px 10px; | ||
| } | ||
| `; |
There was a problem hiding this comment.
The custom CSV table lacks overflow handling. The StyledDocumentViewerContainer has overflow:auto for other renderers (line 52), but StyledCsvTable doesn't inherit this behavior. For CSVs with many columns or wide content, the table may overflow the container without scrollbars. Consider wrapping the table in a scrollable container or adding overflow styles to handle wide tables.
| useEffect(() => { | ||
| if (fileExtension === 'csv') { | ||
| fetchCsvPreview(documentUrl).then((content) => { | ||
| setCsvPreview(content); | ||
| }); | ||
| fetchCsvPreview(documentUrl).then(setCsvPreview); | ||
| } | ||
| }, [documentUrl, fileExtension]); |
There was a problem hiding this comment.
The useEffect hook that calls fetchCsvPreview does not handle errors. If the fetch or parsing fails, the csvPreview state will remain undefined, and users will see the "Loading csv ..." message indefinitely. Consider adding error handling with try-catch and displaying an appropriate error message to users when CSV loading fails.
| th, | ||
| td { | ||
| padding: 5px 10px; |
There was a problem hiding this comment.
The CSV table cells render raw text without any sanitization or escaping. While React automatically escapes text content to prevent XSS, consider edge cases where cell content might contain very long strings without spaces, which could break the table layout. Adding word-break or overflow-wrap styles to table cells would prevent layout issues.
| padding: 5px 10px; | |
| padding: 5px 10px; | |
| word-break: break-word; | |
| overflow-wrap: anywhere; |
| <thead> | ||
| <tr> | ||
| {csvPreview.headers.map((header, columnIndex) => ( | ||
| <th key={columnIndex}>{header}</th> |
There was a problem hiding this comment.
The table headers and cells lack proper ARIA attributes or semantic attributes that could enhance accessibility. While the basic table structure is semantically correct, consider whether headers should have scope attributes (scope="col") to explicitly indicate they're column headers, especially for screen reader users navigating complex CSV data.
| <th key={columnIndex}>{header}</th> | |
| <th key={columnIndex} scope="col"> | |
| {header} | |
| </th> |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/twenty-front/src/modules/activities/files/components/DocumentViewer.tsx">
<violation number="1" location="packages/twenty-front/src/modules/activities/files/components/DocumentViewer.tsx:199">
P2: CSV preview state isn’t reset on documentUrl change, so switching between CSVs can show stale table data until the new fetch resolves or fails.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| fetchCsvPreview(documentUrl).then((content) => { | ||
| setCsvPreview(content); | ||
| }); | ||
| fetchCsvPreview(documentUrl).then(setCsvPreview); |
There was a problem hiding this comment.
P2: CSV preview state isn’t reset on documentUrl change, so switching between CSVs can show stale table data until the new fetch resolves or fails.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-front/src/modules/activities/files/components/DocumentViewer.tsx, line 199:
<comment>CSV preview state isn’t reset on documentUrl change, so switching between CSVs can show stale table data until the new fetch resolves or fails.</comment>
<file context>
@@ -175,15 +192,11 @@ export const DocumentViewer = ({
- fetchCsvPreview(documentUrl).then((content) => {
- setCsvPreview(content);
- });
+ fetchCsvPreview(documentUrl).then(setCsvPreview);
}
}, [documentUrl, fileExtension]);
</file context>
| fetchCsvPreview(documentUrl).then(setCsvPreview); | |
| setCsvPreview(undefined); | |
| fetchCsvPreview(documentUrl).then(setCsvPreview); |
|
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:24705 This environment will automatically shut down when the PR is closed or after 5 hours. |
1 participant
Fix issue #10920
The root cause is that
@cyntler/react-doc-viewer's CSV renderer uses cell values as React keys instead of indices.There are two approaches:
@cyntler/react-doc-viewerto fix it.Changes
fetchCsvPreview.ts— Fetches CSV and parses it into headers and rowsDocumentViewer.tsx— Renders CSV with a custom table instead of passing it to DocViewer