Lazy bag loading + improved empty-result UX and i18n#22
Merged
Conversation
Replace `BagSource.data: Uint8Array` with a lazy `read(offset, length)` random-access primitive backed by `Blob.slice()` so multi-GB bags no longer require a single contiguous allocation. The 512 MB hard limit in fileAdapter is removed — peak memory now scales with the chunk being parsed instead of the full file size. ROS1 reindex (full-file scan) is the only remaining path that materializes the whole buffer. Surface core errors through a new `BagLoadError(code, params)` resolved to translated strings at the UI boundary, and show the filename in the loading panel so users can confirm which file is being read. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
When a bag/MCAP loads successfully but contains no rosout or diagnostics topics, the previous flow silently rendered an empty UI — looking like the upload was reverted. Surface the result instead: an amber notice naming the file, plus an expandable list of every topic actually present (topic name + type) so users can see what's in the file. Loaders now collect `availableTopics: TopicInfo[]` for both ROS1 and ROS2 paths and the "no rosout/diagnostics topics" error throw is removed — that case is no longer an error, just an empty result. Includes unit tests covering the populated-and-empty cases for both formats, e2e coverage with two new fixtures (test_sample_no_rosout.bag generated under ros:noetic and a programmatically-built test_sample_no_rosout.mcap via @mcap/core). Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Test Results90 tests 90 ✅ 0s ⏱️ Results for commit 998df39. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Pull request overview
This PR improves large-file handling and user feedback in the browser-based bag analyzer by introducing lazy/random-access bag reads, structured i18n-ready loader errors, and a clearer “loaded but empty” UI state that lists available topics.
Changes:
- Replace
BagSource.data: Uint8Arraywith lazyBagSource.read(offset, length)+sizeto avoid full-file allocation on indexed load paths. - Introduce
BagLoadError(code, params)and translate core errors at the UI boundary; show filename in the loading panel. - Add “loaded but empty” notice (rosout/diagnostics absent) and return/display
availableTopicsfrom ROS1 bag + MCAP loaders, with new unit + e2e coverage.
Reviewed changes
Copilot reviewed 11 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/web/i18n.ts | Adds new loading/error/status strings (filename-aware loading + empty-result messaging). |
| src/web/fileAdapter.ts | Wraps browser File into lazy BagSource via slice().arrayBuffer(). |
| src/web/fileAdapter.test.ts | Updates tests for lazy BagSource behavior and range reads. |
| src/web/App.tsx | Updates upload/export error handling to use BagLoadError, adds filename loading text, and renders empty-result panel with topic listing. |
| src/core/types.ts | Redefines BagSource as lazy and adds BagLoadError + TopicInfo. |
| src/core/rosbagUtils.ts | Updates ROS1 loader to use lazy reads, returns availableTopics, and throws BagLoadError for structured errors. |
| src/core/rosbagUtils.test.ts | Updates fixtures/helpers for lazy BagSource and adds availableTopics test coverage. |
| src/core/mcapUtils.ts | Adds lazy IReadable adapter, returns availableTopics, and updates indexed/streaming logic. |
| e2e/fixtures/test_sample_no_rosout.mcap | Adds MCAP fixture with no rosout/diagnostics topics for empty-result testing. |
| e2e/fixtures/test_sample_no_rosout.bag | Adds ROS1 bag fixture with no rosout/diagnostics topics for empty-result testing. |
| e2e/fixtures/generate_no_rosout_mcap.ts | Adds generator script for the no-rosout MCAP fixture. |
| e2e/fixtures/generate_no_rosout_bag.py | Adds generator script for the no-rosout ROS1 bag fixture. |
| e2e/empty-result.spec.ts | Adds Playwright coverage for the loaded-but-empty UX for both MCAP and ROS1 bag. |
- Fix awkward "Failed to load bag file.: <reason>" punctuation in load and export error messages by stripping the trailing `.`/`。` from the base translation when a detail follows. - Preserve the original error as `cause` on `BagLoadError` so reindex failures don't lose stack/context (extends BagLoadError to accept ErrorOptions-style cause). - Stop the indexed-MCAP fallback heuristic from defeating lazy loading when a file has only unrelated topics: track total Message records iterated by McapIndexedReader and fall back to streaming only when zero records were seen at all (the unchunked-MCAP case), not when zero rosout/diagnostics were collected. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Codex review on PR #22 flagged that the lazy BagSource refactor materially changes the contract documented in ADR 0008 (`{ name, data: Uint8Array }`, no streaming, fileToBagSource calls File.arrayBuffer, 512 MB guard). Capture the rationale for the new lazy `{ name, size, read(offset, length) }` shape, the requirement that triggered it (1 GB+ bags), and the explicit list of paths that still legitimately materialize the full buffer (reindex, outer zstd, unchunked MCAP streaming fallback). Mark the affected sections of ADR 0008 as superseded; the core/web split itself stays in force. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
BagSource.data: Uint8Arraywith a lazyread(offset, length)primitive backed byBlob.slice(). Multi-GB bags no longer require a single contiguous allocation; the 512 MB hard limit is removed and peak memory now scales with the chunk being parsed. ROS1 reindex (full-file scan) is the only remaining path that materializes the whole buffer.BagLoadError(code, params), and show the filename in the loading panel.Test plan
npm run lint— cleannpm run build— cleannpm run test(vitest) — 90/90 pass, including newavailableTopicscases for both ROS1 bag and MCAP plus the no-rosout fixturesnpm run test:e2e(playwright) — 114/114 pass, including newe2e/empty-result.spec.tscovering MCAP and ROS1 bag fixtures🤖 Generated with Claude Code