feat: Add missing reply_to_stream_id field to TextStreamInfo#202
feat: Add missing reply_to_stream_id field to TextStreamInfo#202Soralsei wants to merge 6 commits into
Conversation
…ard it from proto::DataStream.text_header().reply_to_stream_id()
|
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 1 potential issue.
⚠️ 1 issue in files not directly in the diff
⚠️ Writer's metadata accessor never reports the reply-to identifier that was actually sent (src/data_stream.cpp:289)
The writer's local metadata object is never updated with the reply-to value (info_.reply_to_stream_id is left as std::nullopt at src/data_stream.cpp:289), even though the value is stored internally and sent on the wire, so callers inspecting the writer's metadata always see an empty reply-to field.
Impact: Any code that reads TextStreamWriter::info().reply_to_stream_id gets std::nullopt regardless of what was passed to the constructor.
Writer constructor never copies reply_to_id into the TextStreamInfo member
The TextStreamWriter constructor at src/data_stream.cpp:279-290 receives reply_to_id and stores it in the base-class field reply_to_id_ (used by ensureHeaderSent() at src/data_stream.cpp:227-228 to set the proto). It then calls fillBaseInfo(info_, ...) at line 289, which only populates BaseStreamInfo fields (src/data_stream.cpp:86-95). The new TextStreamInfo::reply_to_stream_id field added by this PR at include/livekit/data_stream.h:67 is never assigned in the writer path.
On the reader side, makeTextInfo at src/room_proto_converter.cpp:618-620 correctly populates the field from the proto header. The asymmetry means the writer's info() accessor (include/livekit/data_stream.h:236) always returns a TextStreamInfo with reply_to_stream_id == std::nullopt.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…ctor in data stream tests
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…d .joinable() guard before trying to join the thread
|
Hey @Soralsei, thanks for the contribution! Will get back to you ASAP on feedback. We have some instability right now with PlatformAudio tanking CI runs, please re-run your actions when convenient as needed (sorry, working on that fix as well). |
alan-george-lk
left a comment
alan-george-lk
left a comment
There was a problem hiding this comment.
Overall looks good: just a few nitpicks and curiosity comments to address
| /// IDs of any attached streams (for attached files). | ||
| std::vector<std::string> attachments; | ||
| /// If this stream is a reply to another stream, this field holds its ID | ||
| std::optional<std::string> reply_to_stream_id = std::nullopt; |
There was a problem hiding this comment.
Nit: repo convention (and also elsewhere in this file) is to not default assign std::nullopt, is a bit redundant
There was a problem hiding this comment.
+1, also aligns with the rest of the code base
| { | ||
| ByteStreamWriter writer(*lockLocalParticipant(*sender_room), /*name=*/"payload.bin", topic); | ||
| writer.write(payload); | ||
| writer.close(); | ||
| } |
There was a problem hiding this comment.
Can you explain why this is scoped like this?
Also consider locking the local participant once in this test, unless there's some logical reason not to
There was a problem hiding this comment.
No particular reason except visual separation between sender/receiver logic. Removed in the latest commit
| const std::string& senderIdentity() const { return sender_identity_; } | ||
|
|
||
| private: | ||
| std::thread recv_thread_{}; |
There was a problem hiding this comment.
Nit: can drop {} here, and we tend to prefer initializer lists in constructors vs. inline
stephen-derosa
left a comment
stephen-derosa
left a comment
There was a problem hiding this comment.
overall looks pretty good, one small comment
| /// IDs of any attached streams (for attached files). | ||
| std::vector<std::string> attachments; | ||
| /// If this stream is a reply to another stream, this field holds its ID | ||
| std::optional<std::string> reply_to_stream_id = std::nullopt; |
There was a problem hiding this comment.
+1, also aligns with the rest of the code base
4 participants
Add missing
reply_to_stream_idfield toTextStreamInfoand forward it fromproto::DataStream.text_header().reply_to_stream_id()inroom_proto_converter.cpp:makeTextInfo(...)