fix(slack-work-apps): add attachment content to slack messages#2317
fix(slack-work-apps): add attachment content to slack messages#2317miles-kt-inkeep merged 3 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: ecc8704 The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(2) Total Issues | Risk: Low
🟠⚠️ Major (1) 🟠⚠️
Inline Comments:
- 🟠 Major:
dispatcher.ts:52-65Type definition duplicated inline instead of importing exportedSlackAttachmentinterface
🟡 Minor (1) 🟡
Inline Comments:
- 🟡 Minor:
app-mention.ts:312Debug log at INFO level should be DEBUG
💭 Consider (2) 💭
💭 1) utils.ts:513-526 Interface placement in utils.ts vs types.ts
Issue: The SlackAttachment interface is defined in utils.ts, but other Slack-related interfaces (SlackCommandPayload, SlackCommandResponse) are in services/types.ts.
Why: Co-locating the type with formatAttachments() that uses it is also a valid pattern. This is a developer preference call.
Fix: Either move to types.ts for consistency with other Slack types, or keep near usage. Both are reasonable.
Refs: types.ts — other Slack interfaces
💭 2) events.test.ts Missing edge case test for incomplete attachment fields
Issue: No test verifies that attachment fields with missing title or value are correctly skipped by formatAttachments().
Why: The implementation guards with if (field.title && field.value) but there's no test protecting this behavior from accidental changes.
Fix: Consider adding:
it('should skip fields with missing title or value', () => {
const result = formatAttachments([{
text: 'Message',
fields: [{ title: 'OnlyTitle' }, { value: 'OnlyValue' }]
}]);
expect(result).not.toContain('OnlyTitle');
expect(result).not.toContain('OnlyValue');
});Refs: utils.ts:552-558 — field guard logic
💡 APPROVE WITH SUGGESTIONS
Summary: This is a well-implemented fix that properly adds attachment content (forwarded/shared messages) to the agent's query context. The code is clean, well-tested, and follows existing patterns. The main suggestion is to consolidate the duplicate type definition by importing the existing SlackAttachment interface instead of redefining it inline. The logging level change is a minor polish item.
Discarded (4)
| Location | Issue | Reason Discarded |
|---|---|---|
app-mention.ts:332 |
Context tag naming follows convention | Positive observation, not actionable |
events.test.ts |
Missing test for thread path + attachments | Indirectly covered by getThreadContext with attachments unit test |
events.test.ts |
Missing test for text vs fallback preference | Too minor, behavior is correct and standard Slack pattern |
utils.ts:513-526 |
Missing JSDoc for subset selection | Nice-to-have but not required, type purpose is clear from usage |
Reviewers (4)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-consistency |
4 | 0 | 1 | 0 | 2 | 0 | 1 |
pr-review-tests |
4 | 0 | 1 | 0 | 0 | 0 | 3 |
pr-review-types |
2 | 0 | 0 | 0 | 0 | 0 | 2 |
| Total | 10 | 0 | 2 | 0 | 2 | 0 | 6 |
Note: Type duplication finding was raised by both consistency and types reviewers — merged and routed to inline comment.
| // Has query → Execute agent with streaming | ||
| let queryText = text; | ||
| const attachmentContext = formatAttachments(attachments); | ||
| logger.info({ attachmentContext }, 'Attachment context'); |
There was a problem hiding this comment.
🟡 Minor: Debug log at INFO level
Issue: This log statement outputs the full attachment context at INFO level, which may produce verbose output in production.
Why: Similar context-related logging elsewhere in the codebase uses logger.debug (e.g., checkIfBotThread in utils.ts). INFO level logs are typically reserved for significant operational events, not intermediate processing state.
Fix:
| logger.info({ attachmentContext }, 'Attachment context'); | |
| logger.debug({ attachmentContext }, 'Attachment context'); |
Refs:
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
There was a problem hiding this comment.
PR Review Summary
(2) Total Issues | Risk: Medium
🔴❗ Critical (1) ❗🔴
Inline Comments:
- 🔴 Critical:
dispatcher.ts:52Missing import forSlackAttachmenttype — will cause TypeScript compilation error
🕐 Pending Recommendations (1)
- 🟡
app-mention.ts:312Debug log at INFO level should be DEBUG
🚫 REQUEST CHANGES
Summary: The delta addressed the prior Major finding (replaced inline type with SlackAttachment), but the fix is incomplete — the import statement for SlackAttachment was not added. This will cause a TypeScript compilation error. Add import type { SlackAttachment } from './services/events/utils'; to the imports at the top of dispatcher.ts.
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator |
1 | 0 | 0 | 0 | 1 | 1 | 0 |
| Total | 1 | 0 | 0 | 0 | 1 | 1 | 0 |
Note: Delta review — only dispatcher.ts changed since last review. Sub-agents not dispatched for minimal delta.
| bot_id?: string; | ||
| subtype?: string; | ||
| edited?: unknown; | ||
| attachments?: SlackAttachment[]; |
There was a problem hiding this comment.
🔴 CRITICAL: Missing import for SlackAttachment
Issue: The type SlackAttachment is used here but is not imported. This will cause a TypeScript compilation error: "Cannot find name 'SlackAttachment'".
Why: The prior review suggested replacing the inline type definition with the exported SlackAttachment interface, which was done. However, the corresponding import statement was not added.
Fix: Add the import at the top of the file (after line 14):
import type { SlackAttachment } from './services/events/utils';Refs:
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
All prior feedback has been addressed in this commit:
| Prior Issue | Status |
|---|---|
🔴 Critical: Missing SlackAttachment import in dispatcher.ts |
✅ Resolved — import added on line 14 |
🟡 Minor: Debug log at INFO level in app-mention.ts |
✅ Resolved — log statement removed |
✅ APPROVE
Summary: The latest commit (ecc87043c feedback) correctly addresses both prior review findings. The SlackAttachment type is now properly imported from ./services/events/utils in dispatcher.ts, and the verbose INFO-level log statement has been removed from app-mention.ts. The PR is ready to merge. 🎉
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: Delta review — both changed files (dispatcher.ts, app-mention.ts) contained only fixes for prior review feedback. No new issues introduced.
Reflects PR #2317 which adds support for Slack attachment content (shared/forwarded messages) in the context sent to agents.
Ito Test Report ✅17 test cases ran. 17 passed. All test cases for PR #2317 (add attachment content to Slack messages) passed verification. The tests validate the new ✅ Passed (17)
📋 View Recording |
1 participant
No description provided.