-
Notifications
You must be signed in to change notification settings - Fork 925
fix: Improve WebSocket image frame parsing and validation #1244
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Refactored image frame metadata extraction to use DataView for stride, width, and height. Added validation for frame size, stride, and dimensions, and improved pixel extraction logic to handle non-contiguous row data. Errors are now logged for invalid frames.
WalkthroughThe change enhances frame data parsing in socket communication by introducing validation guards for metadata size, width, and height, replacing manual metadata extraction with a DataView-based approach, validating stride correctness, and implementing conditional pixel buffer allocation with row-by-row copying when stride differs from expected row size. Changes
Sequence DiagramsequenceDiagram
participant Socket as Socket
participant Handler as onmessage Handler
participant Validator as Validation Layer
participant Buffer as Buffer Processing
participant ImageData as ImageData
Socket->>Handler: Receive Frame
Handler->>Validator: Check metadata size ≥ 12 bytes
alt Metadata too small
Validator->>Handler: Log error & return
else Valid metadata size
Validator->>Validator: Extract width, height, stride via DataView
Validator->>Validator: Validate width > 0 and height > 0
alt Invalid dimensions
Validator->>Handler: Log error & return
else Valid dimensions
Validator->>Validator: Validate stride (non-zero, ≥ width×4)
alt Invalid stride
Validator->>Handler: Log error & return
else Valid stride
Buffer->>Buffer: Check if stride == expected row size
alt Stride matches
Buffer->>Buffer: Direct pixel buffer allocation
else Stride differs
Buffer->>Buffer: Row-by-row copy with stride adjustment
end
Buffer->>ImageData: Construct with width, height, pixel data
ImageData->>Handler: Emit {width, ImageData}
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The change involves multiple validation layers, DataView operations, conditional buffer allocation logic, and stride-aware copying, requiring careful verification of correctness across different stride scenarios and edge cases. Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/desktop/src/utils/socket.ts (1)
65-79
: Well-implemented pixel buffer handling.The conditional logic correctly optimizes for the common case (no stride padding) while properly handling the stride case with row-by-row copying. The bounds are safe given the prior validations.
Consider adding a brief comment documenting the RGBA format assumption (4 bytes per pixel) at line 47, as this constant is used throughout the stride calculations:
const source = clamped.subarray(0, metadataOffset); +// Assuming RGBA format: 4 bytes per pixel const expectedRowBytes = width * 4;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src/utils/socket.ts
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}
: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by runningpnpm format
.Use strict TypeScript and avoid any; leverage shared types
Files:
apps/desktop/src/utils/socket.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}
: Use kebab-case for filenames for TypeScript/JavaScript modules (e.g.,user-menu.tsx
).
Use PascalCase for React/Solid components.
Files:
apps/desktop/src/utils/socket.ts
apps/desktop/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/desktop/src/**/*.{ts,tsx}
: Desktop icons are auto-imported (unplugin-icons); do not import icons manually
Desktop IPC: Call generated tauri_specta commands/events; listen to generated events and use typed interfaces
Use @tanstack/solid-query for server state in the desktop app
Files:
apps/desktop/src/utils/socket.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (2)
apps/desktop/src/utils/socket.ts (2)
46-63
: Excellent stride validation logic.The bounds checking correctly ensures that:
- Stride is at least as large as the row size
- Sufficient source data exists for all rows
- Both contiguous and strided cases are handled safely
The implementation properly handles non-contiguous row data and prevents buffer overruns.
81-82
: Correct ImageData construction and callback.The final ImageData is properly constructed from the validated and processed pixel buffer, and the callback receives the expected structure.
Refactored image frame metadata extraction to use DataView for stride, width, and height. Added validation for frame size, stride, and dimensions, and improved pixel extraction logic to handle non-contiguous row data.
Fixes issue where editor was not showing updated frames when changing settings.
Summary by CodeRabbit