-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Fix CSV preview duplicate key warning (#10920) #17754
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,6 +1,9 @@ | ||||||||||
| import { PREVIEWABLE_EXTENSIONS } from '@/activities/files/const/previewable-extensions.const'; | ||||||||||
| import { downloadFile } from '@/activities/files/utils/downloadFile'; | ||||||||||
| import { fetchCsvPreview } from '@/activities/files/utils/fetchCsvPreview'; | ||||||||||
| import { | ||||||||||
| type CsvPreviewData, | ||||||||||
| fetchCsvPreview, | ||||||||||
| } from '@/activities/files/utils/fetchCsvPreview'; | ||||||||||
| import { getFileType } from '@/activities/files/utils/getFileType'; | ||||||||||
| import DocViewer, { DocViewerRenderers } from '@cyntler/react-doc-viewer'; | ||||||||||
| import '@cyntler/react-doc-viewer/dist/index.css'; | ||||||||||
|
|
@@ -79,6 +82,18 @@ const StyledTitle = styled.div` | |||||||||
| font-weight: ${({ theme }) => theme.font.weight.semiBold}; | ||||||||||
| `; | ||||||||||
|
|
||||||||||
| const StyledCsvTable = styled.table` | ||||||||||
| width: 100%; | ||||||||||
| text-align: left; | ||||||||||
| border-collapse: collapse; | ||||||||||
| color: ${({ theme }) => theme.font.color.primary}; | ||||||||||
|
|
||||||||||
| th, | ||||||||||
| td { | ||||||||||
| padding: 5px 10px; | ||||||||||
| } | ||||||||||
| `; | ||||||||||
|
Comment on lines
+85
to
+95
|
||||||||||
|
|
||||||||||
| type DocumentViewerProps = { | ||||||||||
| documentName: string; | ||||||||||
| documentUrl: string; | ||||||||||
|
|
@@ -165,7 +180,9 @@ export const DocumentViewer = ({ | |||||||||
| }: DocumentViewerProps) => { | ||||||||||
| const { t } = useLingui(); | ||||||||||
| const theme = useTheme(); | ||||||||||
| const [csvPreview, setCsvPreview] = useState<string | undefined>(undefined); | ||||||||||
| const [csvPreview, setCsvPreview] = useState<CsvPreviewData | undefined>( | ||||||||||
| undefined, | ||||||||||
| ); | ||||||||||
|
|
||||||||||
| const { extension } = getFileNameAndExtension(documentName); | ||||||||||
| const fileExtension = isDefined(documentExtension) | ||||||||||
|
|
@@ -175,15 +192,11 @@ export const DocumentViewer = ({ | |||||||||
| const isPreviewable = PREVIEWABLE_EXTENSIONS.includes(fileExtension); | ||||||||||
| const isMsOfficeFile = MS_OFFICE_EXTENSIONS.includes(fileExtension); | ||||||||||
|
|
||||||||||
| const mimeType = PREVIEWABLE_EXTENSIONS.includes(fileExtension) | ||||||||||
| ? MIME_TYPE_MAPPING[fileExtension] | ||||||||||
| : undefined; | ||||||||||
| const mimeType = isPreviewable ? MIME_TYPE_MAPPING[fileExtension] : undefined; | ||||||||||
|
|
||||||||||
| useEffect(() => { | ||||||||||
| if (fileExtension === 'csv') { | ||||||||||
| fetchCsvPreview(documentUrl).then((content) => { | ||||||||||
| setCsvPreview(content); | ||||||||||
| }); | ||||||||||
| fetchCsvPreview(documentUrl).then(setCsvPreview); | ||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: The promise returned by Suggested FixAdd a Prompt for AI AgentDid we get this right? 👍 / 👎 to inform future reviews.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Suggested change
|
||||||||||
| } | ||||||||||
| }, [documentUrl, fileExtension]); | ||||||||||
|
Comment on lines
197
to
201
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CSV preview stuck loading
Prompt To Fix With AIThis 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.
Comment on lines
197
to
201
|
||||||||||
|
|
||||||||||
|
|
@@ -218,12 +231,37 @@ export const DocumentViewer = ({ | |||||||||
| ); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| if (fileExtension === 'csv' && !isDefined(csvPreview)) | ||||||||||
| if (fileExtension === 'csv') { | ||||||||||
| if (!isDefined(csvPreview)) { | ||||||||||
| return ( | ||||||||||
| <StyledDocumentViewerContainer> | ||||||||||
| <Trans>Loading csv ... </Trans> | ||||||||||
| </StyledDocumentViewerContainer> | ||||||||||
| ); | ||||||||||
| } | ||||||||||
| return ( | ||||||||||
| <StyledDocumentViewerContainer> | ||||||||||
| <Trans>Loading csv ... </Trans> | ||||||||||
| <StyledCsvTable> | ||||||||||
| <thead> | ||||||||||
| <tr> | ||||||||||
| {csvPreview.headers.map((header, columnIndex) => ( | ||||||||||
| <th key={columnIndex}>{header}</th> | ||||||||||
|
||||||||||
| <th key={columnIndex}>{header}</th> | |
| <th key={columnIndex} scope="col"> | |
| {header} | |
| </th> |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2,21 +2,22 @@ import Papa from 'papaparse'; | |||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| const DEFAULT_PREVIEW_ROWS = 50; | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| export const fetchCsvPreview = async (url: string): Promise<string> => { | ||||||||||||||||||||||||||||||||
| export type CsvPreviewData = { | ||||||||||||||||||||||||||||||||
| headers: string[]; | ||||||||||||||||||||||||||||||||
| rows: string[][]; | ||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| export const fetchCsvPreview = async (url: string): Promise<CsvPreviewData> => { | ||||||||||||||||||||||||||||||||
| const response = await fetch(url); | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
| 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}`, | |
| ); | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Copilot
AI
Feb 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Copilot
AI
Feb 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Copilot
AI
Feb 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.