Add browser-based rosbag reindex for unindexed bag files#15
Merged
Conversation
Unindexed bag files (e.g. from crashed recordings) previously showed an error asking users to run `rosbag reindex` on the command line. Now the reindex is performed automatically in the browser by scanning chunks and rebuilding the index binary structure. - Add reindexUtils.ts: binary-level bag format parser and index rebuilder - Auto-reindex on load with notification banner and download button - Pass existing ArrayBuffer to reindexer to avoid redundant file reads - Add e2e tests for unindexed and truncated bag files (10 tests) - Add i18n keys (EN/JA) for reindex notification UI Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Test Results60 tests 60 ✅ 0s ⏱️ Results for commit c3e18eb. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Pull request overview
Adds a client-side ROS1 rosbag “reindex in browser” path so unindexed/crash-recovered .bag files can be opened instead of immediately erroring, and exposes a UI prompt to download the reindexed output.
Changes:
- Introduces
reindexUtils.tsto scan chunks and rebuild ROS1 bag index structures fully in the browser. - Updates rosbag loading to detect unindexed bags, reindex them in-memory, and keep a reindexed blob available for download.
- Adds UI + i18n strings + Playwright coverage (with fixtures) for reindex behavior and truncated-bag resilience.
Reviewed changes
Copilot reviewed 7 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/reindexUtils.ts | New binary-level ROS1 bag scanner/index rebuilder used for browser reindexing. |
| src/rosbagUtils.ts | Detects unindexed bags, triggers in-browser reindex, stores reindexed blob metadata for UI download. |
| src/App.tsx | Adds reindex notice banner and download button wiring. |
| src/i18n.ts | Adds EN/JA translation keys for the reindex notice and download CTA. |
| e2e/reindex.spec.ts | Adds Playwright coverage for unindexed-bag load/reindex notice/download + truncated-bag “no crash”. |
| e2e/fixtures/test_unindexed.bag | Fixture bag without index section for e2e validation. |
| e2e/fixtures/test_truncated.bag | Fixture bag truncated mid-chunk to simulate crashed recording. |
| e2e/fixtures/generate_unindexed_bag.py | Script to generate the unindexed bag fixture via ROS tooling. |
| e2e/fixtures/create_unindexed_bag.ts | Node-based script to generate unindexed + truncated fixtures from test_sample.bag. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… index entries - Return reindexedBlob in loadRosbagMessages result instead of module-level mutable state (getReindexedBag/clearReindexedBag removed) - Add downloadBlob() for Blob downloads, reuse in downloadFile() to deduplicate download logic while keeping type safety - Use subarray() instead of slice() for chunk bytes to avoid extra copy - Sort IndexData entries by timestamp before writing for spec compliance Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 9 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Show a clear error message when a 0-byte file is uploaded instead of silently failing with no feedback. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
When a file over 512 MB fails with NotReadableError, display file size and explain the browser memory limitation instead of the generic browser error message. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
- Empty file (0-byte .bag and .mcap) throws "Empty file" error - Large file (>512MB) with NotReadableError shows file size in message - Small file with NotReadableError passes through original error Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
- Guard extractFields() against infinite loop on zero-length fields - Fix buildHeaderBytes() buffer size to use encoded byte length - Release original bag/reader references after reindex to reduce peak memory - Remove unused isUnindexedBag() export and dead code Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 10 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Release arrayBuffer and reader references after reindex to reduce peak memory - Hoist TextDecoder/TextEncoder to module level to reduce GC pressure - Make DecompressFn size parameter optional to match actual handler signatures Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Replace string-based truncation checks with typed errors, expose structured recovery blockers in the UI, and stabilize reindex tests around explicit selectors and metadata.
…t tests - Add defensive check for negative dataLen in buildBagHeaderRecord (C1) - Add sanity check for compressed chunks reporting decompressed size 0 (C2) - Add 8 direct unit tests for reindexBagFromBuffer including roundtrip (I1) - Wrap reindex block in try-catch with contextual error message (I2) - Move isReindexFailureLike to reindexUtils.ts alongside ReindexFailureError (I3) - Export and deduplicate assertNever from reindexUtils.ts (I4) - Add readonly to ReindexFailureError.blockers (I5) Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Skip message indices whose connection metadata could not be recovered so partial reindex output remains reopenable by @foxglove/rosbag. Add targeted warning coverage for missing connection metadata and compressed chunk recovery failures.
- Narrow scanChunkData catch to TruncatedRecordError and RangeError only; unexpected errors (TypeError etc.) now propagate instead of being masked as data corruption (H1) - Add name check to isReindexFailureLike for stronger duck typing (H2) - Preserve original stack trace via error.cause in reindex error wrapper (S1) - Deduplicate blocker display: show concise i18n message instead of raw error with embedded blocker list (S2) - Add readonly to ReindexWarning fields (S3) - Add chunk-record-corrupt warning test with garbage bytes in chunk (S4) Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 4.6 (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
reindexUtils.tsscans chunks and rebuilds the bag index binary structure entirely client-sideChanges
src/reindexUtils.ts— Binary-level ROS1 bag format parser and index rebuildersrc/rosbagUtils.ts— Auto-reindex on unindexed bag detection, reindexed blob storage for downloadsrc/App.tsx— Reindex notification UI with download buttonsrc/i18n.ts— EN/JA translation keys for reindex notificatione2e/reindex.spec.ts— 10 e2e tests (unindexed bag load, reindex notice, download, filters, truncated bag recovery)e2e/fixtures/— Test fixtures: unindexed bag, truncated bag, generator scriptTest plan
npm run test— 42 unit tests passnpm run lint— Zero warningsnpm run build— Build succeedsnpx playwright test e2e/reindex.spec.ts— 10 reindex e2e tests passnpx playwright test e2e/app.spec.ts— 50 existing e2e tests pass (no regressions)🤖 Generated with Claude Code