Develop#1
Conversation
Handle mutex poisoning in file_transferer by returning MapError. Replace fragile unwraps with match/error paths for camera open/read, VideoWriter::fourcc, and chrono::Duration::from_std. Reset jitter buffer state when the read slot is empty to avoid panics. Tidy imports and perform minor UI/test cleanups.
Add doc comments for FileTransferer, SCTPTransport, Controller APIs, ICE agent, and server/user handler flows. Clarify method responsibilities (new, send/accept/reject file, connect, open/accept data channel). No functional changes; minor import reordering.
Add #[must_use] to FTPMessage methods, make AudioFrame::new a const fn,
derive Eq for Frame, simplify pattern matches and struct initialization,
use digit separators in numeric literals, and standardize panic/log
formatting to use {var}. Also add AlgorithmHint to cvt_color call and
fix some OpenCV/FPS type conversions and minor control-flow/formatting
issues.
Introduce FileTransfererNetParams and FileTransfererRuntime to group FileTransferer parameters and simplify construction. Remove Logger from DataChannel API and from various transport call sites. Add ConnectionContext to central_server to pass shared server state. Misc: string/formatting cleanup, timeout casts, and other minor ergonomic fixes.
Add doc comments to many modules (media, file transfer, SCTP, controller, server, session) and add/adjust #[must_use] annotations. Clarify error conditions and return values for several APIs.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly improves the project's documentation, test coverage, and code structure. Key updates include translating the README to English with added architecture diagrams, refactoring the SCTP transport and file transfer modules for better modularity, and introducing comprehensive unit tests for audio codecs, protocol serialization, and clock management. Feedback focuses on removing conversational artifacts from the documentation and correcting logic errors in the newly added audio unit tests.
| - `src/bin/server/main.rs`: punto de entrada del servidor | ||
| - `src/bin/client/main.rs`: punto de entrada del cliente | ||
| - `docs/Informe.md`: informe tecnico del proyecto | ||
| *** Let me know if you'd like to tweak any other part of the documentation! |
There was a problem hiding this comment.
This line appears to be a conversational artifact, possibly from a generation tool. It should be removed to maintain a professional tone in the project's documentation.
| let garbage_payload = vec![0u8, 255, 12, 33]; | ||
|
|
||
| let result = decoder.decode(&garbage_payload); | ||
| assert!(result.is_ok(), "Debería fallar con datos basura"); |
There was a problem hiding this comment.
The assertion result.is_ok() contradicts the failure message "Debería fallar con datos basura" (It should fail with garbage data). When decoding invalid data, an error is expected. The test should assert that result.is_err() is true. Additionally, the message should be in English to align with the rest of the project's new documentation language.
assert!(
result.is_err(),
"Decoding garbage data should result in an error"
);| #[test] | ||
| fn test_encode_buffer_size_constraint() { | ||
| let mut encoder = AudioEncoder::new().unwrap(); | ||
| let frame = create_dummy_frame(1, 0.0); | ||
|
|
||
| let result = encoder.encode(frame); | ||
|
|
||
| let _ = result; | ||
| } |
There was a problem hiding this comment.
This test doesn't perform any assertions. It calls encoder.encode with a frame containing a single sample, which is not a valid Opus frame size and should result in an error. The test should assert that an error is returned to verify the encoder's behavior with invalid input.
| #[test] | |
| fn test_encode_buffer_size_constraint() { | |
| let mut encoder = AudioEncoder::new().unwrap(); | |
| let frame = create_dummy_frame(1, 0.0); | |
| let result = encoder.encode(frame); | |
| let _ = result; | |
| } | |
| #[test] | |
| fn test_encode_buffer_size_constraint() { | |
| let mut encoder = AudioEncoder::new().unwrap(); | |
| let frame = create_dummy_frame(1, 0.0); | |
| let result = encoder.encode(frame); | |
| assert!(result.is_err(), "Encoding a frame with an invalid sample count should fail."); | |
| } |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if let Some(rx) = &self.file_send_rx | ||
| && let Ok(Some(path)) = rx.try_recv() | ||
| { | ||
| if let Err(e) = self.controller.send_file(path.as_path()) { | ||
| self.warning_msg = Some(e.to_string()); | ||
| } | ||
| self.file_send_rx = None; | ||
| } |
There was a problem hiding this comment.
🟡 file_send_rx never cleared when user cancels the file picker dialog
The refactoring from nested if let to chained let patterns changed the cleanup behavior. When the user cancels the file picker, pick_file_to_send() sends None through the channel. The old code matched on Ok(result) and unconditionally set self.file_send_rx = None regardless of whether result was Some or None. The new code uses Ok(Some(path)) as the pattern, which does not match Ok(None), so the block is never entered and self.file_send_rx remains Some(disconnected_receiver) indefinitely. On subsequent frames, try_recv() returns Err(Disconnected), which also doesn't match, so the stale receiver is never cleaned up until replaced by clicking "Send file" again.
| if let Some(rx) = &self.file_send_rx | |
| && let Ok(Some(path)) = rx.try_recv() | |
| { | |
| if let Err(e) = self.controller.send_file(path.as_path()) { | |
| self.warning_msg = Some(e.to_string()); | |
| } | |
| self.file_send_rx = None; | |
| } | |
| if let Some(rx) = &self.file_send_rx | |
| && let Ok(result) = rx.try_recv() | |
| { | |
| if let Some(path) = result { | |
| if let Err(e) = self.controller.send_file(path.as_path()) { | |
| self.warning_msg = Some(e.to_string()); | |
| } | |
| } | |
| self.file_send_rx = None; | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
| let parsed = FTPMessage::from_bytes(&bytes).expect("should parse FileChunk"); | ||
|
|
||
| match parsed { | ||
| FTPMessage::FileChunk { payload } => assert_eq!(payload, payload), |
There was a problem hiding this comment.
🟡 Test asserts payload equals itself due to variable shadowing
In file_chunk_roundtrip_and_malformed, the outer payload is moved into FTPMessage::FileChunk { payload } at line 131. In the match arm at line 137, payload is the destructured field from the parsed message, which shadows the (already moved) outer variable. assert_eq!(payload, payload) compares this variable with itself, so the assertion trivially passes regardless of whether serialization/deserialization is correct — the test does not actually validate the roundtrip.
Prompt for agents
In src/file/file_transferer/ftp_message.rs, the test file_chunk_roundtrip_and_malformed at line 137 has assert_eq!(payload, payload) which compares the destructured variable with itself. Fix this by keeping a clone of the original payload before moving it into the FTPMessage, then comparing against that clone in the match arm. For example, change line 130 to let original_payload = vec![1u8, 2, 3, 4, 5]; then line 131 to let chunk = FTPMessage::FileChunk { payload: original_payload.clone() }; and line 137 to FTPMessage::FileChunk { payload } => assert_eq!(payload, original_payload).
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
Uh oh!
There was an error while loading. Please reload this page.