Conversation
There was a problem hiding this comment.
Pull request overview
Updates Slack /list rendering to display parsed ticket IDs (from work_items.ticket_ids) as a [id1,id2] prefix on each item, while avoiding duplicate prefixes when the description already starts with the same ticket bracket.
Changes:
- Update
/listitem rendering to use a ticket-aware description formatter. - Add helper functions to canonicalize ticket ID lists and detect leading
[tickets]prefixes to prevent duplication. - Add unit tests covering list description formatting and duplicate-prefix behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/integrations/slack/slack.go | Renders /list items using formatItemDescriptionForList, with canonicalization and leading-prefix detection. |
| internal/integrations/slack/slack_logic_test.go | Adds unit tests for ticket-prefix formatting and duplicate-prefix handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func formatItemDescriptionForList(item WorkItem) string { | ||
| description := strings.TrimSpace(item.Description) | ||
| tickets := canonicalTicketList(item.TicketIDs) | ||
| if tickets == "" { | ||
| return description | ||
| } |
There was a problem hiding this comment.
formatItemDescriptionForList trims item.Description and returns the trimmed value even when there are no ticket IDs (or when the prefix is already present). Previously /list rendered item.Description verbatim, so this subtly changes output formatting and also contradicts the PR note about keeping existing formatting for items without ticket IDs. Consider keeping the original description for the no-ticket / already-prefixed cases (use a trimmed copy only for comparisons), and only trim when you actually need to construct a new prefixed string.
| }, | ||
| want: "[7003004] Add schema validation guard", | ||
| }, | ||
| { |
There was a problem hiding this comment.
The new tests cover single-ID and single-prefix dedup, but they don't cover the case this code explicitly supports where the existing leading prefix contains multiple IDs and/or spaces (e.g. description starts with "[7003002, 7003003] ..." while TicketIDs is "7003002,7003003"). Adding a test case for that would better pin the intended canonicalization/dedup behavior.
| { | |
| { | |
| name: "no duplicate when description has multi-id spaced prefix", | |
| item: WorkItem{ | |
| Description: "[7003002, 7003003] Improve cache warm-up sequence", | |
| TicketIDs: "7003002,7003003", | |
| }, | |
| want: "[7003002, 7003003] Improve cache warm-up sequence", | |
| }, | |
| { |
1 participant
What changed
Tests