Skip to content

Fix multi-statement result export - #628

Open
DhruvShah-Dev wants to merge 4 commits into
TabularisDB:mainfrom
DhruvShah-Dev:fix/export-multistatement-results
Open

Fix multi-statement result export#628
DhruvShah-Dev wants to merge 4 commits into
TabularisDB:mainfrom
DhruvShah-Dev:fix/export-multistatement-results

Conversation

@DhruvShah-Dev

Copy link
Copy Markdown
Contributor

Summary

  • enable export for active multi-statement result tabs
  • export loaded multi-statement result rows directly so temp-table scripts can be saved without rerunning on a new connection
  • select the first result-bearing statement after batch execution and add formatter coverage for CSV, JSON, and Markdown exports

Fixes #627

Validation

  • pnpm.cmd exec vitest run tests/utils/resultExport.test.ts tests/utils/multiResult.test.ts
  • pnpm.cmd typecheck
  • pnpm.cmd lint

@DhruvShah-Dev
DhruvShah-Dev marked this pull request as ready for review August 13, 2026 00:40
@kilo-code-bot

kilo-code-bot Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Incremental review of commit b3541fe ("Warn on truncated loaded result exports") since the previous review at ac19202. The change adds a warning in the export progress modal when only the already-loaded rows of a paginated result are exported.

Files Reviewed (6 files)
  • src/components/modals/ExportProgressModal.tsx
  • src/pages/Editor.tsx
  • src/utils/resultExport.ts
  • tests/components/modals/ExportProgressModal.test.tsx
  • tests/utils/resultExport.test.ts
  • tests/setup.ts

(12 i18n locale files also received a matching exportLoadedRowsWarning key.)

The logic is sound: getLoadedRowsExportLimit guards on non-finite total_rows and only warns when loadedRows < totalRows; the warning is preserved through the completed state via partial state updates and correctly suppressed on the error path. Test coverage is adequate.

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)
  • src/pages/Editor.tsx
  • src/utils/resultExport.ts
  • tests/utils/resultExport.test.ts

Reviewed by glm-5.2 · Input: 83.9K · Output: 7.3K · Cached: 274.4K

@debba debba left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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 lowercase null in 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.
  2. There's some overlap with the CSV helpers in src/utils/clipboard.ts, but your csvValue actually 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Export button is disabled when executing multiple statements (e.g. CREATE TEMP TABLE)

2 participants