Repository navigation
Conversation
Code Metrics Report
Details | | main (b7769c5) | #545 (ffe90ac) | +/- |
|---------------------|----------------|----------------|-------|
+ | Coverage | 88.9% | 88.9% | +0.0% |
| Files | 209 | 216 | +7 |
| Lines | 6318 | 6451 | +133 |
+ | Covered | 5618 | 5741 | +123 |
- | Test Execution Time | 2m23s | 2m32s | +9s |Code coverage of files in pull request scope (92.4% → 92.5%, patch 91.1%)Reported by octocov |
IzumiSy
changed the title
|
/review |
|
✅ Code Review completed successfully!
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Generated by Code Review for #545 · copilot · gpt54 · 103.6 AIC · ⌖ 5.49 AIC · ⊞ 15.4K
Comment /review to run again
interacsean
left a comment
interacsean
left a comment
There was a problem hiding this comment.
I can happily admit this might be an old way of thinking, but I am used to seeing this sort of function — creation of a csv — happen serverside and returned as a file over the wire. Times may have changed 😅
Since the file is compiled front-end, and each pagination awaits completion of the previous, it is possible that the data on page 2 is different to what it would have been when page 1 was returned.
In many cases this probably is okay, since the pagination is based on cursor ids, we shouldn't drop rows or get duplicates. If records have any sort of zero-sum dependency on each other (imagine inventory of an item across different locations... which as an example, I admit is not going to run into the 1000s), then the data within each row can double-count something that was already reflected on a previous page
| props: CsvExporterProps<TRow>, | ||
| ) { | ||
| const { visibleColumns } = useDataTableContext<TRow>(); | ||
| const t = useCsvExporterT(); |
There was a problem hiding this comment.
I feel like we could pass a lot of known things in. Either in the hook or in the CsvExporterView, to avoid unnecessary duplication of passing in what you need:
- The filters, sort that are selected in the DataTable
- The graphql query that the data-table is using to fetch the data
Much of this would need to be frozen when the export starts, as I understand the user can still browse the table, update filters, sort etc, while the export continues
Without this, the CsvExporter and the DataTable could have out-of-sync props passed to them, and nothing would complain, the resulting csv would be inconsistent with the table data they are seeing
There was a problem hiding this comment.
Added regression coverage and a query-sharing example in f10be88.
useDataTable receives fetched data rather than owning query execution or application-specific filter normalization. The example reuses the same query document, normalized query/order criteria, and connection-to-rows adapter for both useQuery and imperative CSV requests.
The regression test changes both filter and sort while the first page is pending: subsequent pages retain the starting criteria, and the next export uses the updated criteria. This relies on immutable criteria captured by the starting fetcher; mutable external state or latest-value refs are not snapshotted automatically.
Query integration remains application-owned. This documents and tests the supported wiring rather than automatically enforcing that an arbitrary fetcher matches the table's query and conditions.
| pagination: { first, after }, | ||
| }); | ||
| return { | ||
| edges: page.rows.map((node) => ({ node })), |
There was a problem hiding this comment.
Seems unusual to be mapping back to edges ({ node}[])
I think a smoke signal that when called within a useDataTable context, there should be some intelligence to use the same query execution function as the DataTable already uses
There was a problem hiding this comment.
Changed in e0cefe0. CSV fetchers now return { rows, pageInfo, total? }, matching DataTable's row representation instead of requiring Relay-style edge wrappers. The showcase exporter now calls the same queryInvoices function as table loading directly, without the rows-to-edges adapter.
useDataTable receives fetched data rather than owning query execution, so the fetcher remains explicit. This makes reuse straightforward, but does not add automatic query execution through DataTable context.
| try { | ||
| const filename = normalizeFilename(requestedFilename); | ||
| const columns = resolveColumns( | ||
| columnOverride && usesDataTableColumns ? columnOverride : columnSource, |
There was a problem hiding this comment.
Again if the consumer couldn't pass in columns to the DataTable.CsvExporter, then we would be assured that the columns passed as a prop are the columns of DataTable
There was a problem hiding this comment.
Addressed in 383279e. Added useDataTableCsvExporter, which accepts no columns configuration and can be called outside DataTable.Root. DataTable.CSVExporter accepts only its dedicated, privately branded exporter props, so standalone exporter props or controller substitution are rejected by TypeScript.
The component obtains the enclosing table's visible columns and order directly from context when the download starts. That schema stays fixed during the export; subsequent visibility/order changes apply to the next export. Fixed/custom report schemas remain available through standalone useCsvExporter and CsvExporter.
Tests cover the type restrictions, visibility/order changes, in-flight schema stability, and validation before fetching when no exportable columns remain.
|
|
||
| # CsvExporter | ||
|
|
||
| `CsvExporter` is the standalone UI for [`useCsvExporter`](../api/use-csv-exporter.md). It provides a trigger button, editable filename dialog, fetch progress, and cancellation without requiring a DataTable. |
There was a problem hiding this comment.
No longer provides a button
There was a problem hiding this comment.
Fixed in e0cefe0. The docs and component JSDoc now clarify that standalone CsvExporter provides the dialog and progress UI, while the caller supplies a trigger using the hook's open() action. DataTable.CSVExporter includes its own trigger button. The generated docs were regenerated from the authored source.
|
@copilot resolve the merge conflicts in this pull request |
# Conflicts: # docs/components/data-table.md # examples/vite-app/src/App.tsx # examples/vite-app/src/pages/showcase/data-table-lab/page.tsx # examples/vite-app/src/pages/showcase/data-table/page.tsx # packages/core/src/components/data-table/data-table.tsx Co-authored-by: IzumiSy <982850+IzumiSy@users.noreply.github.com>
Resolved in da830f9 by merging current |
|
@copilot resolve the merge conflicts in this pull request |
# Conflicts: # docs-manifest.json Co-authored-by: IzumiSy <982850+IzumiSy@users.noreply.github.com>
Resolved in c9f4fdd by merging latest |
Motivation
List views need a consistent way to download all records matching their active filters and sort order. Implementations previously had to duplicate cursor traversal, export state, CSV serialization, browser download handling, and the feedback required for a long client-side export.
Design Decision
Client boundary
useCsvExporterremains independent of a particular data client. Consumers provide a cursor-pagefetcher, including any client-specific query and error handling, while AppShell owns cursor traversal, cancellation, progress accounting, CSV generation, and download behavior.DataTable reconciliation
DataTable.CSVExporterreads the DataTable context at the moment an export starts. WhenuseCsvExporteris configured with DataTable columns, it reconciles the CSV schema with the user's current table layout: hidden columns are excluded and visible columns are emitted in the user's reordered sequence. This keeps downloaded data aligned with the table the user is inspecting without making the generic hook depend on DataTable.Explicit
CsvExportColumn[]definitions remain authoritative. They are intended for exports that need a report-specific schema or formatting and therefore intentionally ignore per-user table visibility and ordering.Why columns belong to the hook
DataTable.CSVExportercan readvisibleColumns, but it does not own the canonical CSV schema.useCsvExporterstays usable outside a DataTable — for example, behind a regular button or another collection UI — and is where consumers declare whether an export derives raw values from DataTable columns or uses an explicit report schema. Keeping that declaration at the hook boundary also lets TypeScript require anidoraccessorfor every labelled DataTable column before an export can begin.The component applies only the user's transient layout preference — visibility and ordering — to a DataTable-derived schema at export time. It never rewrites an explicit
CsvExportColumn[]report. This keeps data ownership declarative and client-agnostic while making the DataTable integration feel native.Standard and DataTable integration
useCsvExporterfollows the existing CSV importer pattern: it returns anopen()action and apropsobject. The standalone<CsvExporter {...props} />provides the standard filename/progress UI outside a DataTable.<DataTable.CSVExporter {...props} />accepts the same props and adds only the table-context reconciliation described above.Neither component owns data-client details. The toolbar remains normally composable, so consumers choose the DataTable variant's placement with their existing layout rather than a special Toolbar prop.
Trigger
Long-running client exports
DataTable.CSVExporteropens a filename dialog and reports fetched rows, using a determinate progress bar when the connection returnstotal. Users can cancel through the supplied abort signal, or dismiss and reopen the dialog while an export continues. CSV data is UTF-8 BOM encoded and formula-like cells are escaped for spreadsheet safety.Filling a filename
Data fetching progress
Summary
useCsvExporterand its cursor/exporter types.CsvExporterandDataTable.CSVExportervariants with a shared filename/progress UI and current-column reconciliation.