internal(Data files): UI for importing data files #427
Conversation
| @@ -1,2 +1,5 @@ | |||
| // eslint-disable-next-line no-control-regex | |||
| export const INVALID_FILENAME_CHARS = /[<>:"/\\|?*\x00-\x1F]/ | |||
|
|
|||
| // Limit the file size for preview to avoid performance issues | |||
| export const FILE_SIZE_PREVIEW_THRESHOLD = 1024 * 1024 * 10 | |||
There was a problem hiding this comment.
Could probably be higher, I'll revisit this before opening the PR for review.
We don't want to load large files into the main process memory, so similarly to how GitHub doesn't show previews of large files, we'll display a message if the file size is above this threshold
There was a problem hiding this comment.
I'd say github case is a bit different, it doesn't show preview to prevent overloading dom and slowing down the browser, while in our case we only show the first 10 rows. We should probably do some testing with large files and see how it affects electron's memory consumption, verify there's no memory leak, and that it gets cleaned up properly. Not part of this PR though 👍
| columns?: number | ||
| } | ||
|
|
||
| export function TableSkeleton({ |
There was a problem hiding this comment.
Might be an overkill, considering the data loads pretty fast. Depends on whether we increase the file size threshold 🤔
There was a problem hiding this comment.
I think this works great! Had a few minor comments:
-
Empty lines in CSV are parsed as empty rows:
-
If you import a CSV and JSON file with the same name, there's no indication which is which in the sidebar. Perhaps we could include file extension or add a badge, like the one you have in the preview title.
-
Not a
chore😅
| if (size > FILE_SIZE_PREVIEW_THRESHOLD) { | ||
| return null | ||
| } |
There was a problem hiding this comment.
Show an error in the UI indicating file is too large to preview, instead of just "no preview available"?
There was a problem hiding this comment.
Added a warning/info message. Once the layout update PR is merged, I'll probably also add a grot illustration :D
| @@ -1,2 +1,5 @@ | |||
| // eslint-disable-next-line no-control-regex | |||
| export const INVALID_FILENAME_CHARS = /[<>:"/\\|?*\x00-\x1F]/ | |||
|
|
|||
| // Limit the file size for preview to avoid performance issues | |||
| export const FILE_SIZE_PREVIEW_THRESHOLD = 1024 * 1024 * 10 | |||
There was a problem hiding this comment.
I'd say github case is a bit different, it doesn't show preview to prevent overloading dom and slowing down the browser, while in our case we only show the first 10 rows. We should probably do some testing with large files and see how it affects electron's memory consumption, verify there's no memory leak, and that it gets cleaned up properly. Not part of this PR though 👍
Good catch, I'll need to verify how it works in the k6 csv module and match the behavior
Good point, I'll check what works/looks better 👍
Fair enough, it's just that it's currently also not a user-facing feature. Should we add the |
Either that or |
going-confetti
changed the title
| return parseCSV<DataRecord>(content, { | ||
| header: true, | ||
| delimiter: ',', | ||
| skipEmptyLines: true, |
There was a problem hiding this comment.
I checked k6/experimental/csv and it does skip empty lines 👍
I will address this in a separate PR |
Description
How to Test
+in the data files tree in the sidebarI used is this JSON file, it has some nesting so it's good to test how nested properties are displayed
Screenshots (if appropriate):
Related PR(s)/Issue(s)