fix(dingtalk): preserve inbound quoted message context - #9464
Open
chinatsu1124 wants to merge 3 commits into
Open
fix(dingtalk): preserve inbound quoted message context#9464chinatsu1124 wants to merge 3 commits into
chinatsu1124 wants to merge 3 commits into
Conversation
📄 Knowledge reviewDosu skipped reviewing this PR because your organization has used its |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The hard-coded placeholder labels for non-text content (e.g. "[Image]", "[File]", "[Chat history]") could be centralized or made configurable/localizable to avoid scattering user-facing strings inside the adapter.
- Inserting the Reply component at index 0 of
abm.messagechanges the message ordering; if other consumers rely on the first element being the leading At/Plain, consider documenting this behavior or appending the Reply and handling ordering at a higher layer. - The timestamp normalization logic in
_parse_replyuses a bare> 10_000_000_000heuristic; extracting this into a small helper or clarifying the expected units (ms vs s) would make the intent easier to understand and maintain.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The hard-coded placeholder labels for non-text content (e.g. "[Image]", "[File]", "[Chat history]") could be centralized or made configurable/localizable to avoid scattering user-facing strings inside the adapter.
- Inserting the Reply component at index 0 of `abm.message` changes the message ordering; if other consumers rely on the first element being the leading At/Plain, consider documenting this behavior or appending the Reply and handling ordering at a higher layer.
- The timestamp normalization logic in `_parse_reply` uses a bare `> 10_000_000_000` heuristic; extracting this into a small helper or clarifying the expected units (ms vs s) would make the intent easier to understand and maintain.
## Individual Comments
### Comment 1
<location path="astrbot/core/platform/sources/dingtalk/dingtalk_adapter.py" line_range="111" />
<code_context>
return dingtalk_id[len(prefix) :]
return dingtalk_id or "unknown"
+ def _parse_reply(
+ self,
+ message: dingtalk_stream.ChatbotMessage,
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring `_parse_reply` by extracting repeated defensive dict handling and quote-processing into small helper methods to simplify the control flow and clarify responsibilities.
You can reduce complexity by extracting the repeated defensive patterns and separating responsibilities into small helpers, while preserving behaviour.
### 1. Encapsulate defensive dict access
The `getattr` + `isinstance` pattern is repeated; a small helper keeps the flow cleaner:
```python
def _safe_dict(self, obj: object | None) -> dict:
return obj if isinstance(obj, dict) else {}
```
Then at the top of `_parse_reply`:
```python
text_content = getattr(message, "text", None)
text_extensions = self._safe_dict(getattr(text_content, "extensions", None))
message_extensions = self._safe_dict(getattr(message, "extensions", None))
```
### 2. Split `_parse_reply` into focused helpers
You can keep `_parse_reply` as an orchestration method and move the logic into small helpers:
```python
def _extract_quote(self, text_ext: dict, msg_ext: dict) -> dict | None:
replied_message = text_ext.get("repliedMsg")
legacy_quote = msg_ext.get("quoteMessage")
if text_ext.get("isReplyMsg") and isinstance(replied_message, dict):
return replied_message
if isinstance(legacy_quote, dict):
return legacy_quote
return None
```
```python
def _extract_quoted_text(self, quote: dict) -> str:
message_type = str(quote.get("msgType") or quote.get("msgtype") or "text").strip()
content = quote.get("content")
if not isinstance(content, (dict, str)):
content = quote.get("text")
if isinstance(content, str):
content = {"text": content}
if not isinstance(content, dict):
content = {}
# main text / content keys
for key in ("text", "content"):
value = content.get(key)
if isinstance(value, str) and value.strip():
return value.strip()
# richText handling
if message_type == "richText":
parts: list[str] = []
rich_text = content.get("richText")
if isinstance(rich_text, list):
for item in rich_text:
if not isinstance(item, dict):
continue
item_text = item.get("content") or item.get("text")
if isinstance(item_text, str) and item_text.strip():
parts.append(item_text.strip())
elif (item.get("msgType") or item.get("type")) == "picture":
parts.append("[Image]")
return "".join(parts)
# generic placeholders / file
placeholders = {
"picture": "[Image]",
"audio": "[Audio]",
"voice": "[Audio]",
"video": "[Video]",
"interactiveCard": "[Card]",
"chatRecord": "[Chat history]",
}
if message_type == "file":
file_name = content.get("fileName")
if isinstance(file_name, str) and file_name:
return f"[File: {file_name}]"
return "[File]"
return placeholders.get(message_type, "")
```
```python
def _extract_quote_metadata(self, quote: dict, msg_ext: dict) -> tuple[str, str, int]:
quote_id = str(quote.get("msgId") or msg_ext.get("originalMsgId") or "").strip()
sender_id = str(quote.get("senderId") or "")
if sender_id:
sender_id = self._id_to_sid(sender_id)
created_at = quote.get("createdAt") or 0
try:
quote_time = int(created_at)
if quote_time > 10_000_000_000:
quote_time //= 1000
except (TypeError, ValueError):
quote_time = 0
return quote_id, sender_id, quote_time
```
Then `_parse_reply` becomes a linear, high-level flow:
```python
def _parse_reply(
self,
message: dingtalk_stream.ChatbotMessage,
) -> Reply | None:
text_content = getattr(message, "text", None)
text_extensions = self._safe_dict(getattr(text_content, "extensions", None))
message_extensions = self._safe_dict(getattr(message, "extensions", None))
quote = self._extract_quote(text_extensions, message_extensions)
if quote is None:
return None
quoted_text = self._extract_quoted_text(quote)
quote_id, sender_id, quote_time = self._extract_quote_metadata(quote, message_extensions)
if not quote_id and not quoted_text:
return None
return Reply(
id=quote_id,
chain=[Plain(quoted_text)] if quoted_text else [],
sender_id=sender_id,
sender_nickname=str(quote.get("senderNick") or ""),
time=quote_time,
message_str=quoted_text,
text=quoted_text,
)
```
This keeps all your current behaviours (including edge-case handling) but lowers nesting, makes responsibilities clearer, and should address the complexity concerns without changing functionality.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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.
Fixes #9463.
DingTalk reply callbacks expose quoted messages through
text.isReplyMsgandtext.repliedMsg, but the adapter previously converted only the new message body. Quoted text was discarded, while quoted images were represented only by placeholders and never reached the model as image components.Modifications / 改动点
Convert current DingTalk
text.repliedMsgpayloads into AstrBotReplycomponents.Retain compatibility with legacy top-level
quoteMessagepayloads.Preserve the quoted message ID, text, sender metadata, and timestamp when available.
Download standalone quoted pictures with
downloadCodeandrobotCode, then attach them asImagecomponents inReply.chain.Preserve the original text/image order for quoted rich-text messages and resolve embedded pictures into
Imagecomponents.Fall back to readable image placeholders when media download information is unavailable or the download fails.
Add adapter tests for current text replies, legacy quote payloads, standalone picture replies, and rich-text replies containing pictures.
This is NOT a breaking change. / 这不是一个破坏性变更。
Screenshots or Test Results / 运行截图或测试结果
The warning is the pre-existing
audioopdeprecation warning.Checklist / 检查清单