Handle detailed DingTalk chatRecord payloads#525
Conversation
Greptile Summary本 PR 将 Confidence Score: 5/5代码逻辑正确,核心场景有测试覆盖,可安全合并;遗留意见均为 P2 级别的改进建议 所有反馈均为 P2 级别:原始条目回退路径的格式问题仅在极罕见的非对象条目场景下出现,rawRecord 重复逻辑目前行为一致,messages 别名缺测试不影响已实现功能的正确性。核心流程均经过测试验证。 src/message-utils.ts 的 formatChatRecordEntries 非对象条目回退路径和 rawRecord 别名重复逻辑需关注 Important Files Changed
Reviews (1): Last reviewed commit: "Handle detailed DingTalk chatRecord payl..." | Re-trigger Greptile |
| if (!entry || typeof entry !== "object") { | ||
| return trimString(String(entry ?? "")); | ||
| } |
There was a problem hiding this comment.
当 entry 是非空的原始值(如字符串 "alice: hello" 或数字)时,此处直接返回 trimString(String(entry)),输出的行没有 发送者: 内容 前缀,与其他条目的格式混合在一起。实际上 DingTalk 载荷几乎不会出现纯字符串条目,但若出现,[聊天记录内容] 段中会夹杂格式不一致的裸字符串。
建议在这种情况下也返回 undefined 跳过该条目:
| if (!entry || typeof entry !== "object") { | |
| return trimString(String(entry ?? "")); | |
| } | |
| if (!entry || typeof entry !== "object") { | |
| return undefined; | |
| } |
| if (msgtype === "chatRecord") { | ||
| const content = data.content as Record<string, unknown> | undefined; | ||
| const summary = typeof content?.summary === "string" ? content.summary.trim() : ""; | ||
| const rawRecord = content?.chatRecord; | ||
| const rawRecord = content?.chatRecord ?? content?.records ?? content?.messages; | ||
| const chatRecordText = formatChatRecordPreview(content); |
There was a problem hiding this comment.
rawRecord 的别名解析链(chatRecord ?? records ?? messages)在这里(用于 guard 判断)与 formatChatRecordPreview 内部的第 183 行各自独立计算。若未来在 formatChatRecordPreview 中调整了字段顺序或增加新别名,而这里的 guard 未同步更新,可能出现 guard 已放行但 chatRecordText 仍为空(或反之)的情况。
可以将 rawRecord 传入 formatChatRecordPreview,或将公共的别名解析提取为共享常量,使两处逻辑保持同源。
| it('top-level chatRecord — expands records aliases instead of only summary', () => { | ||
| const message = { | ||
| msgId: 'test', | ||
| createAt: 0, | ||
| conversationType: '1', | ||
| conversationId: 'cid', | ||
| senderId: 'sid', | ||
| chatbotUserId: 'bot', | ||
| sessionWebhook: 'https://example.com', | ||
| msgtype: 'chatRecord', | ||
| content: { | ||
| summary: '祝欣莹:[消息]\n溯煜:[分享]', | ||
| records: [ | ||
| { senderName: '祝欣莹', content: '第一条' }, | ||
| { senderName: '溯煜', message: '第二条' }, | ||
| ], | ||
| }, | ||
| } as any; | ||
|
|
||
| const content = extractMessageContent(message); | ||
|
|
||
| expect(content.messageType).toBe('chatRecord'); | ||
| expect(content.text).toContain('[聊天记录摘要] 祝欣莹:[消息]'); | ||
| expect(content.text).toContain('[聊天记录内容]'); | ||
| expect(content.text).toContain('祝欣莹: 第一条'); | ||
| expect(content.text).toContain('溯煜: 第二条'); | ||
| }); |
|
Addressed the Greptile review feedback in 95b1a04:\n\n- skip non-object chatRecord entries instead of rendering bare strings\n- moved chatRecord/records/messages alias resolution into a shared helper\n- added coverage for the messages alias\n\nValidation rerun:\n- pnpm exec oxfmt --check src/message-utils.ts\n- pnpm test tests/unit/message-utils.test.ts\n- pnpm type-check\n- pnpm test |
|
Added one more regression test in 221ee8f using the real summary-only DingTalk chatRecord shape. It documents the platform boundary: when DingTalk only sends title + summary and no chatRecord/records/messages entries, the parser preserves the summary but cannot expand missing detailed records.\n\nRerun:\n- pnpm test tests/unit/message-utils.test.ts -- -t 'logged DingTalk summary-only payload'\n- pnpm test tests/unit/message-utils.test.ts\n- pnpm type-check |
zizhus-ai
left a comment
There was a problem hiding this comment.
Code Review
整体改动质量不错,函数拆分合理,测试覆盖充分,向后兼容性好。以下是需要关注的问题:
CRITICAL
stringifyChatRecordContentValue 中 record.content 双重解析逻辑不够清晰
return (
trimString(record.text as string | undefined) ||
trimString(record.content as string | undefined) || // content 当 string
trimString(record.message as string | undefined) ||
trimString((record.content as Record<string, unknown> | undefined)?.text as string | undefined)
// ^^ 如果上一行 content 是非空 string,这行永远不会到达
);record.content 在 || 链中被当作两种不同类型使用(string 和 object),虽然运行时不会崩溃,但语义混乱、可维护性差。建议拆成显式的 if/else 分支。
HIGH
1. rawRecord 别名解析逻辑重复
extractMessageContent 中 L694-695:
const rawRecord = content?.chatRecord ?? content?.records ?? content?.messages;已有 getChatRecordEntriesSource() 封装了相同逻辑,这里应直接调用:
const rawRecord = getChatRecordEntriesSource(content);否则未来新增别名时两处需同步更新,容易遗漏。
2. .slice(0, 30) 魔数
formatChatRecordEntries 末尾的 .slice(0, 30) 缺少说明,建议提取为命名常量:
const MAX_CHAT_RECORD_ENTRIES = 30;
// ...
.slice(0, MAX_CHAT_RECORD_ENTRIES);MEDIUM
"某人"硬编码中文回退值,建议提取为常量便于未来维护options.titlePrefix参数名语义不够清晰,useTitleAsLabel之类的名字更直观- 4 个测试用例的消息骨架字段高度重复,建议提取 factory 函数减少冗余
Positive
- 函数拆分职责清晰:
stringifyChatRecordContentValue,getChatRecordEntriesSource,formatChatRecordEntries,formatChatRecordPreview - summary-only 降级路径保留完好,向后兼容
- 测试覆盖了 summary-only、详细展开、
chatRecord/records/messages三种别名 - formatter 风格调整与逻辑变更分离,易于 review
建议修复 HIGH 问题后合入。
|
已根据最新 review 在 6cdd539 做了补充清理:
本轮没有改动 summary-only 的平台边界行为;DingTalk 只给 title + summary、不提供详细 entries 时仍只保留 summary,不伪造明细。 验证:
|
zizhus-ai
left a comment
There was a problem hiding this comment.
Code Review
整体改动质量不错,函数拆分合理,向后兼容性好,之前的 review 反馈已基本落实。以下是剩余建议:
HIGH
stringifyChatRecordContentValue 字段优先级缺少注释
record.text > content (string 或 .text) > message 的优先级链是正确的,但对后续维护者不够直观。建议在函数顶部加一行注释说明优先级逻辑,避免将来误改顺序。
MEDIUM
1. "[]" 占位符判断散布多处
extractMessageContent 和 formatChatRecordPreview 各自独立检查 summary === "[]"。如果 DingTalk 后续出现其他占位值("null", "{}"),需要同步更新多处。建议提取 isPlaceholderValue(str) 工具函数。
2. 测试 fixture 冗余
4 个测试用例重复了相同的消息骨架字段(msgId, createAt, conversationType 等)。建议提取 buildTestMessage(overrides) 工厂函数,减少噪音、突出测试意图。
3. 测试中 as any 类型断言
所有测试消息均使用 as any。考虑定义 partial type 或 fixture builder 生成正确类型的对象,这样 DingTalkInboundMessage 类型变更时能自动发现不兼容。
Positive
- 函数拆分职责清晰:
stringifyChatRecordContentValue,getChatRecordEntriesSource,formatChatRecordEntries,formatChatRecordPreview - summary-only 降级路径保留完好
getChatRecordEntriesSource消除了别名解析重复MAX_CHAT_RECORD_ENTRIES/UNKNOWN_PERSON_LABEL常量提取到位- 测试覆盖了 summary-only 回归、详细展开、三种别名
结论:之前 review 的问题已修复,剩余为可维护性建议,非正确性问题。Approve。
|
Follow-up for the approved maintainability suggestion in 75feff7:
Validation run locally:
|
Summary
chatRecordpayloads when forwarded record details are present on replied messageschatRecord,records, andmessagesaliases for top-level chat record contentTests
pnpm test tests/unit/message-utils.test.tspnpm type-checkpnpm testpnpm exec oxfmt --check src/message-utils.tsNote:
pnpm lintcurrently reports existing warnings in unrelated files but 0 errors.