fix(export): clamp oversized Lightning media probe ranges#522
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughExtracts HTTP byte-range parsing into exported ChangesHTTP Range Request Parsing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
electron/mediaServer.test.ts (1)
74-98: ⚡ Quick winAdd a regression case for malformed/multi-range headers.
Please add explicit
toBeNull()cases for values likebytes=0-1,2-3andbytes=0-1fooso parser strictness is locked by tests.🧪 Suggested test addition
describe("resolveHttpByteRange", () => { + it("rejects malformed and multi-range headers", async () => { + const { resolveHttpByteRange } = await import("./mediaServer"); + expect(resolveHttpByteRange("bytes=0-1,2-3", 100)).toBeNull(); + expect(resolveHttpByteRange("bytes=0-1foo", 100)).toBeNull(); + }); + it("clamps oversized explicit end offsets to EOF", async () => { const { resolveHttpByteRange } = await import("./mediaServer");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/mediaServer.test.ts` around lines 74 - 98, Add regression tests for malformed or multi-range HTTP Range headers by asserting resolveHttpByteRange returns null for those inputs: call resolveHttpByteRange with "bytes=0-1,2-3" and with "bytes=0-1foo" (and the same EOF values used elsewhere) and add expect(...).toBeNull() assertions; locate where resolveHttpByteRange is imported in the existing tests (the describe block in mediaServer.test.ts) and add these two cases alongside the other it(...) cases so parser strictness is locked by tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@electron/mediaServer.ts`:
- Around line 15-16: The range parsing currently uses an unanchored regex
(rangeHeader.match) which allows malformed or multi-range headers to partially
match; replace the regex with an anchored pattern (for example
/^\s*bytes=(\d*)-(\d*)\s*$/) and keep the existing check that both capture
groups are not empty so headers like "bytes=0-1,2-3" or "bytes=0-1foo" are
rejected rather than partially accepted; update the code around the match
variable and the rangeHeader.match call accordingly so only a full, single-range
header passes.
---
Nitpick comments:
In `@electron/mediaServer.test.ts`:
- Around line 74-98: Add regression tests for malformed or multi-range HTTP
Range headers by asserting resolveHttpByteRange returns null for those inputs:
call resolveHttpByteRange with "bytes=0-1,2-3" and with "bytes=0-1foo" (and the
same EOF values used elsewhere) and add expect(...).toBeNull() assertions;
locate where resolveHttpByteRange is imported in the existing tests (the
describe block in mediaServer.test.ts) and add these two cases alongside the
other it(...) cases so parser strictness is locked by tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: cca9eb93-f741-4442-9a5f-80f6fbbea064
📒 Files selected for processing (2)
electron/mediaServer.test.tselectron/mediaServer.ts
1 participant
Summary
/videomedia serverRoot Cause
Lightning metadata probing goes through the local loopback media server. For large files, the demuxer can request a byte range whose end overshoots the file size. Our server treated that as a hard
416 Range Not Satisfiableinstead of clamping the end to EOF, which could causeget_media_info failed: Failed after 3 attemptsbefore export even started.Impact
This fixes the media-server bug at the source instead of adding another exporter fallback. Large local recordings should now survive oversized probe ranges during Lightning startup.
Testing
npx vitest run electron/mediaServer.test.tsSummary by CodeRabbit