Decouple core bag loaders from browser APIs and split src into core/web#19
Decouple core bag loaders from browser APIs and split src into core/web#19
Conversation
The rosbag/mcap loaders previously depended on the DOM File API and on
web-only BlobReader. This blocks reuse from Node contexts such as a
future TUI. Introduce a platform-agnostic BagSource ({ name, data:
Uint8Array }) as the input to loadMessages/loadRosbagMessages/
loadMcapMessages, return reindexed output as Uint8Array (not Blob), and
move the File-to-BagSource conversion plus the browser download helpers
into a new src/fileAdapter.ts that App.tsx uses directly.
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Physically separate the platform-agnostic parsing logic from the browser-only UI so a future TUI can reuse the core without pulling in React/Tailwind/DOM. This is the layout counterpart to the earlier API decoupling (BagSource etc.): now core and web live in distinct trees and the boundary is visible at the directory level rather than only in type signatures. - src/core/ — rosbagUtils, mcapUtils, reindexUtils, types, and their tests. Accepts BagSource, no DOM imports. - src/web/ — App.tsx, main.tsx, i18n.ts, fileAdapter.ts, index.css, assets/, and the new severityStyles.ts extracted from types.ts. - src/core/types.ts no longer carries Tailwind class strings; those moved to src/web/severityStyles.ts. The unused FilterConfig type is dropped. - AGENTS.md, .github/copilot-instructions.md, docs/testing.md, and index.html updated to reference the new paths. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Record the rationale behind decoupling the bag/MCAP loaders from the browser File API and splitting src/ into core/ and web/. The two implementation commits (e286730, 9a0601b) focused on the what; this ADR captures the why, the rejected alternatives (File pass-through, Filelike streaming, monorepo, keeping Tailwind in types.ts, Blob return), and the invariants future changes need to preserve. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Test Results87 tests +1 87 ✅ +1 0s ⏱️ ±0s Results for commit e31cd04. ± Comparison against base commit e4652c9. This pull request removes 86 and adds 87 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Pull request overview
Decouples the bag/MCAP parsing + filtering + export logic from browser-only APIs by introducing a platform-agnostic BagSource input type and physically splitting the code into src/core/ (reusable logic) and src/web/ (React + DOM adapters).
Changes:
- Replace loader inputs from
FiletoBagSource = { name: string; data: Uint8Array }across core loaders and tests. - Move browser-only glue (File reading, downloads, large-file DOMException handling) into
src/web/fileAdapter.ts; move Tailwind severity/diagnostic class mappings intosrc/web/severityStyles.ts. - Update entrypoint and docs; add ADR 0008 to document the architectural split and invariants.
Reviewed changes
Copilot reviewed 16 out of 20 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/web/severityStyles.ts | UI-only Tailwind class maps split out from core types. |
| src/web/main.tsx | New web entrypoint for React app under src/web/. |
| src/web/index.css | Web-only Tailwind + app CSS moved under src/web/. |
| src/web/i18n.ts | Adds EN/JA dictionaries + useI18n hook for the web UI. |
| src/web/fileAdapter.ts | DOM File → BagSource conversion and download helpers (browser-only). |
| src/web/fileAdapter.test.ts | Unit tests for fileToBagSource error wrapping and byte fidelity. |
| src/web/App.tsx | Updates UI to use BagSource + new download helpers; consumes styles from severityStyles. |
| src/types.ts | Removes the old mixed core+web types/constants file. |
| src/core/types.ts | Introduces BagSource and keeps core-only types/constants. |
| src/core/rosbagUtils.ts | Core loaders now operate on BagSource; replaces BlobReader with an in-memory Uint8Array reader; reindex returns bytes. |
| src/core/rosbagUtils.test.ts | Updates unit tests/fixtures to use BagSource instead of File. |
| src/core/reindexUtils.ts | Reindex API now accepts `ArrayBuffer |
| src/core/reindexUtils.test.ts | Updates reindex tests to validate byte output via a minimal in-memory reader. |
| src/core/mcapUtils.ts | MCAP loader now reads from Uint8Array instead of ArrayBuffer/Blob-based readable. |
| index.html | Points Vite entry to /src/web/main.tsx. |
| docs/testing.md | Updates references to the new src/core/rosbagUtils.ts and test file paths. |
| docs/adr/0008-core-web-split-for-non-browser-reuse.md | Adds ADR documenting the split, rationale, and invariants. |
| AGENTS.md | Updates architecture docs + file paths to reflect core/ vs web/. |
| .github/copilot-instructions.md | Updates review guidance to reflect the new directory structure and test paths. |
Comments suppressed due to low confidence (1)
src/core/rosbagUtils.ts:91
bytesis captured as aconstfromsource.data, so even thoughreader.release()is called after reindexing, the original file buffer is still strongly referenced for the rest of this function (and will coexist withreindexedBytes). This can significantly increase peak memory usage during reindexing and may trigger OOM on large bags.
Consider making bytes a let and explicitly dropping the reference after the reindexed bag is opened (e.g., reassign bytes to an empty Uint8Array, or otherwise ensure the original buffer is no longer referenced) so the original bytes can be GC’d while continuing to parse the reindexed bag.
| export function downloadFile(content: string | Uint8Array, filename: string, type: string) { | ||
| const blob = new Blob([content as unknown as BlobPart], { type }); | ||
| downloadBlob(blob, filename); |
There was a problem hiding this comment.
downloadFile uses a double type-cast (as unknown as BlobPart) even though string and Uint8Array are valid BlobParts. This hides type issues and makes the code harder to maintain; consider constructing the Blob without the cast (or narrowing the union) so the compiler can help you.
There was a problem hiding this comment.
Partially addressed in e31cd04: reduced as unknown as BlobPart to a single as BlobPart cast so the assertion's intent is visible.
I cannot remove the cast entirely. Since TS 5.7, Uint8Array is generic over ArrayBufferLike, while DOM's BlobPart requires Uint8Array<ArrayBuffer>. Without any cast, tsc --strict (we use --strict) emits:
Type 'Uint8Array<ArrayBufferLike>' is not assignable to type 'BlobPart'.
Type 'ArrayBufferLike' is not assignable to type 'ArrayBuffer'.
Type 'SharedArrayBuffer' is not assignable to type 'ArrayBuffer'.
I evaluated alternatives (typed BlobPart[] local, narrowing the parameter to Uint8Array<ArrayBuffer>, copying via new Uint8Array(bytes)); the narrowing option requires propagating the constraint through every caller's return type, and the copy adds an unnecessary full-buffer allocation at download time. The single cast is the lowest-impact fix that keeps the original intent ("pass these bytes through to Blob") explicit.
| export function downloadBytes(bytes: Uint8Array, filename: string, type = 'application/octet-stream') { | ||
| downloadBlob(new Blob([bytes as unknown as BlobPart], { type }), filename); |
There was a problem hiding this comment.
Same as downloadFile: the as unknown as BlobPart cast in downloadBytes is unnecessary for a Uint8Array and weakens type checking. Removing it keeps the code simpler and lets TypeScript catch real issues.
There was a problem hiding this comment.
Same as the downloadFile thread: reduced to a single as BlobPart cast in e31cd04. The cast itself can't go away without either narrowing the parameter to Uint8Array<ArrayBuffer> (which has to ripple up to every caller's return type) or copying the buffer at download time. See the other thread for the full reasoning.
- fileToBagSource: attach the original DOMException as `cause` when wrapping the large-file NotReadableError, so the user-facing message is preserved while debuggers / Sentry can still trace the underlying failure. Add `ES2022.Error` to tsconfig lib for the cause option. - downloadFile / downloadBytes: reduce `as unknown as BlobPart` to a single `as BlobPart` cast. The cast itself is still needed (TS 5.7+ Uint8Array<ArrayBufferLike> vs BlobPart's Uint8Array<ArrayBuffer> mismatch), but the double-cast was hiding intent. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Summary
BagSource = { name: string; data: Uint8Array }as the platform-agnostic input to all bag/MCAP loaders, replacing the browserFileAPI dependencysrc/intosrc/core/(parsing, filtering, export — no DOM/React/Tailwind) andsrc/web/(React UI, download helpers, Tailwind styles)File→BagSourceconversion, download helpers, large-fileDOMExceptionhandling) tosrc/web/fileAdapter.tstypes.tsintosrc/web/severityStyles.ts; drop unusedFilterConfigtypeUint8Array(notBlob) from core; web wraps at download timeMotivation
Enable future reuse of the parsing/filtering/export logic from non-browser contexts (TUI, CLI, Node scripts, Web Workers) without pulling in DOM dependencies or maintaining parallel implementations.
Test plan
npm run lint— zero warningsnpm run test— 87 unit tests pass (includes existing + newfileAdapter.test.ts)npm run build— tsc + vite build succeedsnpm run test:e2e— browser E2E tests pass (file upload, export, reindex download unchanged)src/core/has no imports fromsrc/web/,react, or DOM APIs🤖 Generated with Claude Code