Conversation
There was a problem hiding this comment.
Pull request overview
Adds SQLite export support (via sql.js) for rosout logs and diagnostics, along with unit/E2E test coverage to validate downloaded .sqlite contents.
Changes:
- Add
sql.jsintegration and new SQLite export functions for rosout + diagnostics (including nested diagnostic values). - Update UI to offer SQLite as an export format and add stable
data-testidhooks for E2E. - Add Vitest + Playwright coverage and update docs/configs for the new E2E flow.
Reviewed changes
Copilot reviewed 8 out of 11 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/types/sqljs.d.ts | Adds local TypeScript declarations for sql.js and *.wasm?url imports. |
| src/rosbagUtils.ts | Implements SQLite export logic using sql.js, plus updates download helper to accept binary. |
| src/rosbagUtils.test.ts | Adds unit tests validating SQLite schema/row contents for both exports. |
| src/App.tsx | Adds SQLite export option in UI and adds test IDs used by Playwright. |
| playwright.config.ts | Changes webServer reuse behavior for E2E runs. |
| package.json | Adds sql.js dependency. |
| package-lock.json | Locks sql.js dependency. |
| e2e/sqlite-export.spec.ts | Adds Playwright tests that download SQLite files and assert query results. |
| docs/testing.md | Documents Playwright E2E testing command. |
| README.md | Updates export feature list and adds e2e test command mention. |
| .gitignore | Keeps Playwright artifacts ignored. |
Comments suppressed due to low confidence (1)
src/App.tsx:172
- handleExport is now async, but errors from exportToSQLite/exportDiagnosticsToSQLite (e.g., failing to load the wasm) will become unhandled promise rejections because nothing catches them. Wrap the body in try/catch and surface failures via setError (and/or disable the export buttons while an export is in progress).
const handleExport = async (format: 'csv' | 'json' | 'txt' | 'sqlite') => {
const timestamp = new Date().toISOString().replace(/[:.]/g, '-').slice(0, -5);
let content: string | Uint8Array;
let filename: string;
let type: string;
if (activeTab === 'diagnostics') {
const prefix = 'diagnostics_export';
switch (format) {
case 'csv':
content = exportDiagnosticsToCSV(filteredDiagnostics, timezone);
filename = `${prefix}_${timestamp}.csv`;
type = 'text/csv';
break;
case 'json':
content = exportDiagnosticsToJSON(filteredDiagnostics, timezone);
filename = `${prefix}_${timestamp}.json`;
type = 'application/json';
break;
case 'txt':
content = exportDiagnosticsToTXT(filteredDiagnostics, timezone);
filename = `${prefix}_${timestamp}.txt`;
type = 'text/plain';
break;
case 'sqlite':
content = await exportDiagnosticsToSQLite(filteredDiagnostics, timezone);
filename = `${prefix}_${timestamp}.sqlite`;
type = 'application/vnd.sqlite3';
break;
default: {
const exhaustiveFormat: never = format;
throw new Error(`Unsupported export format: ${exhaustiveFormat}`);
}
}
} else {
switch (format) {
case 'csv':
content = exportToCSV(filteredMessages, timezone);
filename = `rosout_export_${timestamp}.csv`;
type = 'text/csv';
break;
case 'json':
content = exportToJSON(filteredMessages, timezone);
filename = `rosout_export_${timestamp}.json`;
type = 'application/json';
break;
case 'txt':
content = exportToTXT(filteredMessages, timezone);
filename = `rosout_export_${timestamp}.txt`;
type = 'text/plain';
break;
case 'sqlite':
content = await exportToSQLite(filteredMessages, timezone);
filename = `rosout_export_${timestamp}.sqlite`;
type = 'application/vnd.sqlite3';
break;
default: {
const exhaustiveFormat: never = format;
throw new Error(`Unsupported export format: ${exhaustiveFormat}`);
}
}
}
downloadFile(content, filename, type);
};
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const blobPart: BlobPart = content instanceof Uint8Array ? new Uint8Array(content).buffer as ArrayBuffer : content; | ||
| const blob = new Blob([blobPart], { type }); |
There was a problem hiding this comment.
downloadFile converts Uint8Array to a fresh ArrayBuffer via new Uint8Array(content).buffer, which copies the full database into a new buffer. Blob can accept a Uint8Array directly; passing the Uint8Array as-is avoids a potentially large extra allocation during SQLite downloads.
| const blobPart: BlobPart = content instanceof Uint8Array ? new Uint8Array(content).buffer as ArrayBuffer : content; | |
| const blob = new Blob([blobPart], { type }); | |
| const blob = new Blob([content], { type }); |
There was a problem hiding this comment.
Addressed in commit bc938cf. The unnecessary buffer copy has been removed. Note: the current code uses content as unknown as BlobPart cast, but Blob constructor accepts string | Uint8Array directly, so the cast could be simplified to just new Blob([content], { type }).
|
|
||
| type Timezone = 'local' | 'utc'; | ||
|
|
||
| let sqlJsPromise: Promise<SqlJsStatic> | null = null; | ||
| const resolvedSqlWasmUrl = | ||
| typeof window === 'undefined' | ||
| ? new URL('../node_modules/sql.js/dist/sql-wasm.wasm', import.meta.url).pathname |
There was a problem hiding this comment.
Using new URL(...).pathname for the sql-wasm.wasm path is not reliably cross-platform (notably on Windows) and can produce an invalid filesystem path. Prefer fileURLToPath(new URL(...)) when targeting Node, and consider avoiding a runtime dependency on ../node_modules/... if this path can ever be used outside tests.
| type Timezone = 'local' | 'utc'; | |
| let sqlJsPromise: Promise<SqlJsStatic> | null = null; | |
| const resolvedSqlWasmUrl = | |
| typeof window === 'undefined' | |
| ? new URL('../node_modules/sql.js/dist/sql-wasm.wasm', import.meta.url).pathname | |
| import { fileURLToPath } from 'url'; | |
| type Timezone = 'local' | 'utc'; | |
| let sqlJsPromise: Promise<SqlJsStatic> | null = null; | |
| const resolvedSqlWasmUrl = | |
| typeof window === 'undefined' | |
| ? fileURLToPath(new URL('../node_modules/sql.js/dist/sql-wasm.wasm', import.meta.url)) |
There was a problem hiding this comment.
Addressed in commit bc938cf with a custom resolveNodeFilePath helper instead of fileURLToPath from the url module. This avoids importing a Node.js-only module in a codebase that also runs in the browser. The helper handles Windows drive-letter paths (/C:/... → C:/...) so cross-platform behavior is covered.
Summary
Testing