Fix multi-statement result export - #628
Conversation
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Incremental review of commit Files Reviewed (6 files)
(12 i18n locale files also received a matching The logic is sound: Previous Review Summary (commit ac19202)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit ac19202)Status: No Issues Found | Recommendation: Merge Files Reviewed (3 files)
Reviewed by glm-5.2 · Input: 83.9K · Output: 7.3K · Cached: 274.4K |
debba
left a comment
There was a problem hiding this comment.
Thanks for the PR! I checked it out locally and tested it against a real SQLite database, and I want to say upfront that the approach is the right one. Exporting the loaded rows directly makes sense here, since rerunning the script through export_query_to_file would open a fresh connection and the temp tables from the batch session would be gone. The code is clean, tests pass (61/61 on your branch), typecheck and lint are green, and the auto-selection of the first result-bearing statement works nicely.
There is one thing I'd like fixed before merging though.
Silent truncation to the loaded page. Batch execution runs with limit: pageSize (default 100), so each result entry only holds the first page. The new export writes just those loaded rows and then reports "completed". I reproduced it with a script selecting from a 250-row table inside a batch: the exported CSV contains only 100 rows, with no hint that anything is missing. That's arguably worse than the old disabled button, because the user walks away believing they have the full result set. The good news is that pagination.total_rows is already available on the entry (it's used for history right in the same function), so detecting the case is cheap. Even just a warning in the export modal when rows.length < total_rows would be enough for a first pass. Paging through the remaining rows would be even better, but I won't block on that.
A couple of smaller notes, none of them blocking:
- NULL values export differently depending on the path now: the backend CSV writer emits a literal
NULL, while the new client-side formatter emits an empty string (and lowercasenullin Markdown). Same query, different output depending on whether the tab was single or multi statement. Worth aligning at some point, and honestly the empty string is probably the better convention for CSV. - There's some overlap with the CSV helpers in
src/utils/clipboard.ts, but yourcsvValueactually escapes quotes, delimiters and newlines correctly while the clipboard one doesn't, so if anything the consolidation should go in your direction. Fine to leave for a follow-up.
I also verified the happy path from #627 manually (temp table + SELECT in one script): the button is enabled, the export writes the correct rows without rerunning anything, and single-statement tabs still go through the full backend streaming export as before. So once the truncation case is handled, this is good to go.
Summary
Fixes #627
Validation