Merged
Conversation
#476 This module provides a data structure to consolidate campaign delivery information into a summary format suitable for various use cases. * Track total and completed task counts * Group assigned deliveries by rider name * Collect unassigned deliveries separately * Implement Collectable protocol for use with Enum.into/2 * Include delivery details: dropoff name, status, and item names
#476 This function enables finding campaigns whose delivery window ended within a specified time range. It's designed to support a scheduled job that runs every 15 minutes to process recently ended campaigns. The function: - Accepts from_datetime and to_datetime as NaiveDateTime - Uses inclusive bounds (>= and <=) for the time window - Returns a list of Campaign structs without preloads Test coverage includes: - Finding campaigns within the specified window - Returning empty list when no campaigns match
#476 Introduces a new schema and migration to store records of campaign summary messages posted to Slack. This enables tracking which campaigns have already had summaries sent. Schema fields: - slack_channel_id: Target Slack channel - raw_message: The formatted message content - sent_at: When the message was posted - campaign_id: Reference to the associated campaign The migration includes indexes on campaign_id and sent_at for efficient lookups when checking if a summary was already posted or querying by time range.
The "monday + week" combined filter test was failing because campaigns were created with delivery_start at 12:00 UTC, which is in the future when tests run before noon UTC. The riders_latest_campaigns view excludes campaigns where delivery_start >= NOW(), so the rider's latest_campaign was NULL. This caused the "week" filter to exclude the rider even though they had a valid Monday campaign. Change the test helper to use 00:00:00 instead of 12:00:00 for delivery_start, ensuring campaigns are always in the past relative to test execution time.
…heduler This is required to fetch ended campaign in the given time interval and used by Campaign Summary Poster function
Used to create the delivery summary structure by Campaign Summary Poster.
Add name, delivery_start, and delivery_end fields to the struct. Add new/1 function that accepts a campaign and pre-populates these metadata fields from campaign.program.name and timestamps.
Use a consistent timestamp across setup and tests instead of calling NaiveDateTime.utc_now() multiple times
Introduces a GenServer that runs every 15 minutes to identify recently ended campaigns and persist their delivery summaries to the database.
- Automates generation of campaign summaries after deliveries are completed
- Includes a 60–75 minute delay to allow riders time to mark tasks as complete
- Stores summaries for reveiw
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughA GenServer schedules every 15 minutes to find campaigns that ended ~60–75 minutes ago, builds per‑campaign delivery summaries from tasks/riders, upserts a slack message record, posts a formatted Slack payload, and marks messages as sent. Added schema, payload builder, summary struct, scheduler, migration, tests, and CodeRabbit config. Changes
Sequence DiagramsequenceDiagram
participant Scheduler as CampaignSummaryPoster (GenServer)
participant Repo as Database (Ecto/Repo)
participant DeliverySvc as Delivery.campaign_riders_and_tasks
participant Collector as CampaignDeliverySummary (Collectable)
participant SlackApi as Slack API (PayloadBuilder + Poster)
loop every 15 minutes
Scheduler->>Repo: list_campaigns_ended_between(from,to)
Repo-->>Scheduler: campaigns
alt for each campaign
Scheduler->>Repo: preload campaign.program
Scheduler->>DeliverySvc: campaign_riders_and_tasks(campaign)
DeliverySvc-->>Scheduler: tasks + rider info
Scheduler->>Collector: new(campaign)
loop tasks
Scheduler->>Collector: add_task(task)
end
Collector-->>Scheduler: CampaignDeliverySummary
Scheduler->>Repo: insert SlackCampaignSummaryMessage (on_conflict: :nothing)
Repo-->>Scheduler: inserted? / existing
alt newly inserted
Scheduler->>SlackApi: build_delivery_summary(channel, summary)
SlackApi-->>Scheduler: payload
Scheduler->>SlackApi: post_message!(payload)
SlackApi-->>Scheduler: success / error
Scheduler->>Repo: update(sent_at)
else already exists or send failed
Scheduler->>Scheduler: log and continue
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Convert the existing campaign_id index to a unique constraint to support atomic inserts with on_conflict: :nothing.
- Prevents duplicate campaign summaries
- Enables race-condition-free inserts
Formats CampaignDeliverySummary data into Slack messages using Block Kit for campaign completion updates. Changes: - Add build_delivery_summary/2 to PayloadBuilder for Slack Block Kit formatting - Add campaign_id field to CampaignDeliverySummary struct - Introduce Slack.CampaignDeliverySummary module with send_summary/2 Message format includes: - Header with program name and campaign link - Date/time range (same-day vs multi-day formatting) - Total and completed delivery counts - Rider sections with task lists and completion status - Unassigned deliveries section (when applicable)
- Add CampaignSummaryPoster to application supervision tree - Post summary to Slack channel after DB insert - Update sent_at timestamp on successful Slack post - Handle Slack errors gracefully without crashing
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
lib/bike_brigade/delivery/campaign_delivery_summary.ex (2)
36-39: Redundant pattern match in fallback clause.The pattern
%{completed: _completed}on line 39 is unnecessary sincecompletedis always present in the struct. A simple variable binding would suffice.♻️ Simplified fallback clause
defp completed(summary, %{delivery_status: :completed}), do: Map.update!(summary, :completed, &(&1 + 1)) - defp completed(%{completed: _completed} = summary, _task), do: summary + defp completed(summary, _task), do: summary🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/bike_brigade/delivery/campaign_delivery_summary.ex` around lines 36 - 39, The fallback clause for the completed/2 function uses an unnecessary map pattern match (%{completed: _completed}); change it to a simple variable binding to avoid redundant matching—update defp completed(%{completed: _completed} = summary, _task) to defp completed(summary, _task) so the function returns summary when the task isn't :completed.
13-20: Requiresprogrampreload—ensure callers preload it.
new/1accessescampaign.program.name, which will raiseEcto.Association.NotLoadedifprogramisn't preloaded. The current caller (CampaignSummaryPoster) does preload it, but consider adding a guard or documenting this requirement.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/bike_brigade/delivery/campaign_delivery_summary.ex` around lines 13 - 20, The new/1 function assumes campaign.program is loaded and will crash with Ecto.Association.NotLoaded; update new/1 to explicitly check Ecto.assoc_loaded?(campaign.program) (or pattern-match on %Program{} in the function head) and raise a clear ArgumentError instructing callers to preload :program (or alternatively return {:error, ...}) so callers like CampaignSummaryPoster know the requirement; reference the new/1 function and campaign.program in your change.test/bike_brigade/slack_api/payload_builder_test.exs (1)
177-190: Timezone assumption may cause test flakiness.The test hardcodes UTC times and expects specific EDT output (
10:00 AM - 2:00 PM). If the application's configured timezone (America/Toronto) observes DST differently in the test environment or if the configuration changes, this test could fail unexpectedly.Consider using a timezone-aware assertion or mocking the timezone configuration for deterministic results.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/bike_brigade/slack_api/payload_builder_test.exs` around lines 177 - 190, The test assumes a specific timezone conversion for build_delivery_summary_payload which can flake; make the test deterministic by controlling the app timezone or by asserting timezone-agnostic output: either set the timezone config to "America/Toronto" for the test (e.g., Application.put_env(:your_app, :timezone, "America/Toronto") or call the same timezone conversion helper used by build_delivery_summary_payload before building the fixture) or construct campaign times already in the target local zone and assert on the resulting summary_text from build_delivery_summary_payload; reference the fixture(:campaign) creation and the build_delivery_summary_payload call and update the test to mock/override timezone or to compute expected local times dynamically from the UTC inputs before asserting summary_text.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/bike_brigade/campaign_summary_poster.ex`:
- Line 43: The code reads channel_id = campaign.program.slack_channel_id without
checking for nil; add a guard clause in the function that builds/posts the
summary (e.g., inside the CampaignSummaryPoster module/function that assigns
channel_id) to skip processing when campaign.program.slack_channel_id is nil:
return early (or next) and optionally log/debug that the campaign was skipped
due to missing Slack channel instead of attempting to post with a nil
channel_id. Ensure the check references campaign.program.slack_channel_id (or
the local channel_id variable) and prevents the Slack API call that uses
channel_id.
In `@lib/bike_brigade/delivery.ex`:
- Around line 321-326: Change the inclusive lower bound in
list_campaigns_ended_between so the window is exclusive on one side: replace
c.delivery_end >= ^from_utc_datetime with c.delivery_end > ^from_utc_datetime
and keep the upper bound as c.delivery_end <= ^to_utc_datetime; then add a
boundary unit test for Campaign.delivery_end == from_utc_datetime to assert the
record is returned only in the intended window (use the
list_campaigns_ended_between function and Campaign.delivery_end in the test).
In `@lib/bike_brigade/slack_api/payload_builder.ex`:
- Around line 65-69: The header and summary currently interpolate raw cds.name
and task_data.items into mrkdwn; wrap those interpolations with the existing
filter_mrkdwn/1 helper so special characters are escaped. Specifically, update
the header construction (where header = ":bar_chart: #{cds.name} Summary ...")
to use filter_mrkdwn(cds.name) and update the place that inserts task_data.items
(the interpolation referenced around task_data.items) to use
filter_mrkdwn(task_data.items) so both values are escaped for Slack mrkdwn
rendering.
In `@test/bike_brigade/riders/rider_search_test.exs`:
- Around line 125-127: The fixture create_campaign_for_date is creating
datetimes at UTC midnight which become the previous day in America/Toronto and
yield the wrong ISO weekday; change the datetime creation so the stored UTC time
maps to the intended local date—either create the DateTime at noon UTC (so
DateTime.new!(date, 12:00:00, "Etc/UTC")) or create a midnight DateTime in
"America/Toronto" and then shift it to UTC (use DateTime.new!(date, 00:00:00,
"America/Toronto") then shift_zone!/to UTC) so the EXTRACT(ISODOW FROM ? AT TIME
ZONE ?) returns the expected weekday for create_campaign_for_date.
---
Nitpick comments:
In `@lib/bike_brigade/delivery/campaign_delivery_summary.ex`:
- Around line 36-39: The fallback clause for the completed/2 function uses an
unnecessary map pattern match (%{completed: _completed}); change it to a simple
variable binding to avoid redundant matching—update defp completed(%{completed:
_completed} = summary, _task) to defp completed(summary, _task) so the function
returns summary when the task isn't :completed.
- Around line 13-20: The new/1 function assumes campaign.program is loaded and
will crash with Ecto.Association.NotLoaded; update new/1 to explicitly check
Ecto.assoc_loaded?(campaign.program) (or pattern-match on %Program{} in the
function head) and raise a clear ArgumentError instructing callers to preload
:program (or alternatively return {:error, ...}) so callers like
CampaignSummaryPoster know the requirement; reference the new/1 function and
campaign.program in your change.
In `@test/bike_brigade/slack_api/payload_builder_test.exs`:
- Around line 177-190: The test assumes a specific timezone conversion for
build_delivery_summary_payload which can flake; make the test deterministic by
controlling the app timezone or by asserting timezone-agnostic output: either
set the timezone config to "America/Toronto" for the test (e.g.,
Application.put_env(:your_app, :timezone, "America/Toronto") or call the same
timezone conversion helper used by build_delivery_summary_payload before
building the fixture) or construct campaign times already in the target local
zone and assert on the resulting summary_text from
build_delivery_summary_payload; reference the fixture(:campaign) creation and
the build_delivery_summary_payload call and update the test to mock/override
timezone or to compute expected local times dynamically from the UTC inputs
before asserting summary_text.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3b1a5a6d-01bc-4c59-8d8d-514976fb9519
📒 Files selected for processing (13)
.coderabbit.yamllib/bike_brigade/application.exlib/bike_brigade/campaign_summary_poster.exlib/bike_brigade/delivery.exlib/bike_brigade/delivery/campaign_delivery_summary.exlib/bike_brigade/messaging/slack.exlib/bike_brigade/messaging/slack_campaign_summary_message.exlib/bike_brigade/slack_api/payload_builder.expriv/repo/migrations/20260329221529_add_slack_campaign_summary_messages.exstest/bike_brigade/delivery/campaign_delivery_summary_test.exstest/bike_brigade/delivery_test.exstest/bike_brigade/riders/rider_search_test.exstest/bike_brigade/slack_api/payload_builder_test.exs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/bike_brigade/slack_api/payload_builder.ex`:
- Around line 112-137: The section text can exceed Slack's 3,000-character limit
because format_task_lines returns an unbounded string; modify build_rider_block
and build_unassigned_block to enforce a max payload length before embedding
task_lines: compute available_space = 3000 - byte_size(header_text) -
safety_margin (e.g. 100), then if byte_size(task_lines) > available_space
truncate it safely (String.slice or :binary.part to preserve UTF-8) and append a
clear suffix like "… (truncated)" or "… (+N more)"; keep references to
format_task_lines (source of the lines) and filter_mrkdwn (used for rider_name)
so truncation happens after formatting but before constructing the Slack block
to prevent invalid_blocks errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 120b1746-b6e5-4d69-8ab5-38fb11681b47
📒 Files selected for processing (1)
lib/bike_brigade/slack_api/payload_builder.ex
Comment on lines
+112
to
+137
| defp build_rider_block({rider_name, tasks}) do | ||
| total = length(tasks) | ||
| completed = Enum.count(tasks, &(&1.delivery_status == :completed)) | ||
| status_text = "(#{completed}/#{total})" | ||
| task_lines = format_task_lines(tasks) | ||
|
|
||
| %{ | ||
| type: "section", | ||
| text: %{ | ||
| type: "mrkdwn", | ||
| text: ":bicyclist: *#{filter_mrkdwn(rider_name)}* #{status_text}\n#{task_lines}" | ||
| } | ||
| } | ||
| end | ||
|
|
||
| defp build_unassigned_block([]), do: [] | ||
|
|
||
| defp build_unassigned_block(unassigned_tasks) do | ||
| task_lines = format_task_lines(unassigned_tasks) | ||
|
|
||
| [ | ||
| %{type: "divider"}, | ||
| %{ | ||
| type: "section", | ||
| text: %{type: "mrkdwn", text: ":package: *Unassigned Deliveries*\n#{task_lines}"} | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the maximum character length for Slack Block Kit section text objects (mrkdwn/plain_text), and what error response does chat.postMessage return when that limit is exceeded?
💡 Result:
-
Max character length (Section block
textobject,mrkdwnorplain_text): 3,000 characters (min 1, max 3000). The Block Kit Text object (type: mrkdwn|plain_text) itself has a 3000-character max, and the Section blocktextfield references that same limit. [1], [2] -
chat.postMessageerror when you exceed it (e.g., sectiontext.text> 3000): Slack rejects the blocks as invalid and returns an error JSON like:{ "ok": false, "error": "invalid_blocks" }[3]
Sources:
[1] Slack “Text object” (Block Kit composition object) docs
[2] Slack “Section block” docs
[3] Slack chat.postMessage docs (Errors: invalid_blocks)
🏁 Script executed:
cat -n lib/bike_brigade/slack_api/payload_builder.ex | head -150Repository: bikebrigade/dispatch
Length of output: 5143
🏁 Script executed:
cat -n lib/bike_brigade/slack_api/payload_builder.ex | tail -50Repository: bikebrigade/dispatch
Length of output: 1594
Guard against Slack section text length overflows for large task lists.
format_task_lines concatenates all tasks without bounds. A rider with many deliveries or long dropoff/item names can cause task_lines to exceed Slack's 3,000-character section text limit, resulting in chat.postMessage failing with {"ok": false, "error": "invalid_blocks"} and dropping the entire summary message.
Implement truncation with a safety margin to prevent this:
Suggested hardening (truncate before building section text)
+ `@slack_section_text_safe_limit` 2900
+
defp build_rider_block({rider_name, tasks}) do
total = length(tasks)
completed = Enum.count(tasks, &(&1.delivery_status == :completed))
status_text = "(#{completed}/#{total})"
- task_lines = format_task_lines(tasks)
+ task_lines = tasks |> format_task_lines() |> clamp_slack_text()
%{
type: "section",
text: %{
type: "mrkdwn",
text: ":bicyclist: *#{filter_mrkdwn(rider_name)}* #{status_text}\n#{task_lines}"
}
}
end
defp build_unassigned_block(unassigned_tasks) do
- task_lines = format_task_lines(unassigned_tasks)
+ task_lines = unassigned_tasks |> format_task_lines() |> clamp_slack_text()
[
%{type: "divider"},
%{
type: "section",
text: %{type: "mrkdwn", text: ":package: *Unassigned Deliveries*\n#{task_lines}"}
}
]
end
+
+ defp clamp_slack_text(text) when is_binary(text) do
+ if String.length(text) > `@slack_section_text_safe_limit` do
+ String.slice(text, 0, `@slack_section_text_safe_limit`) <> "\n…"
+ else
+ text
+ end
+ endAlso applies to: 141-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/bike_brigade/slack_api/payload_builder.ex` around lines 112 - 137, The
section text can exceed Slack's 3,000-character limit because format_task_lines
returns an unbounded string; modify build_rider_block and build_unassigned_block
to enforce a max payload length before embedding task_lines: compute
available_space = 3000 - byte_size(header_text) - safety_margin (e.g. 100), then
if byte_size(task_lines) > available_space truncate it safely (String.slice or
:binary.part to preserve UTF-8) and append a clear suffix like "… (truncated)"
or "… (+N more)"; keep references to format_task_lines (source of the lines) and
filter_mrkdwn (used for rider_name) so truncation happens after formatting but
before constructing the Slack block to prevent invalid_blocks errors.
- Return early with warning log when slack_channel_id is nil - Update list_campaigns_ended_between docs to clarify (from, to] bounds
4 tasks
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/bike_brigade/campaign_summary_poster.ex (1)
70-79: Reasonable error handling, but failed sends won't be retried.When Slack sending fails, the record remains in the database without
sent_at, but due toon_conflict: :nothing, it won't be automatically retried on the next scheduler run. This is acceptable for an initial implementation, but consider adding a separate job to retry records wheresent_at IS NULLif delivery reliability is important.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/bike_brigade/campaign_summary_poster.ex` around lines 70 - 79, The current send_and_mark_sent function logs failures from send_to_slack but leaves records with nil sent_at that will never be retried due to on_conflict: :nothing; add a retry mechanism by enqueuing a background job (e.g., create or call a RetrySlackSummary job) whenever send_to_slack returns {:error, reason} instead of just logging, and ensure mark_as_sent(record) is only called on :ok; update send_and_mark_sent to call the job enqueuer with the record id (or relevant payload) on error so records with sent_at IS NULL get retried later.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/bike_brigade/campaign_summary_poster.ex`:
- Around line 70-79: The current send_and_mark_sent function logs failures from
send_to_slack but leaves records with nil sent_at that will never be retried due
to on_conflict: :nothing; add a retry mechanism by enqueuing a background job
(e.g., create or call a RetrySlackSummary job) whenever send_to_slack returns
{:error, reason} instead of just logging, and ensure mark_as_sent(record) is
only called on :ok; update send_and_mark_sent to call the job enqueuer with the
record id (or relevant payload) on error so records with sent_at IS NULL get
retried later.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 46cf1dc0-fed3-45f8-8abf-179a6ebb08ee
📒 Files selected for processing (2)
lib/bike_brigade/campaign_summary_poster.exlib/bike_brigade/delivery.ex
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/bike_brigade/delivery.ex
Documented modules:
- CampaignSummaryPoster: GenServer that schedules summary posting
- CampaignDeliverySummary: Struct for aggregating delivery statistics
- Slack.CampaignDeliverySummary: Sends summaries to Slack channels
- SlackCampaignSummaryMessage: Schema for tracking sent messages
- PayloadBuilder.build_delivery_summary/2: Builds Block Kit payloads
Also uses %__MODULE__{} struct pattern match in CampaignDeliverySummary completed/2 for more precise type matching.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/bike_brigade/slack_api/payload_builder.ex (1)
123-148:⚠️ Potential issue | 🟠 MajorBound
task_linesbefore embedding in section blocks.
format_task_lines/1is unbounded, so large campaigns can overflow Slack section text limits and cause message rejection (invalid_blocks). This can drop the whole summary post.Proposed hardening diff
+ `@slack_section_text_safe_limit` 2900 + defp build_rider_block({rider_name, tasks}) do total = length(tasks) completed = Enum.count(tasks, &(&1.delivery_status == :completed)) status_text = "(#{completed}/#{total})" - task_lines = format_task_lines(tasks) + task_lines = tasks |> format_task_lines() |> clamp_slack_text() %{ type: "section", text: %{ type: "mrkdwn", text: ":bicyclist: *#{filter_mrkdwn(rider_name)}* #{status_text}\n#{task_lines}" } } end defp build_unassigned_block(unassigned_tasks) do - task_lines = format_task_lines(unassigned_tasks) + task_lines = unassigned_tasks |> format_task_lines() |> clamp_slack_text() [ %{type: "divider"}, %{ type: "section", text: %{type: "mrkdwn", text: ":package: *Unassigned Deliveries*\n#{task_lines}"} } ] end + + defp clamp_slack_text(text) when is_binary(text) do + if String.length(text) > `@slack_section_text_safe_limit` do + String.slice(text, 0, `@slack_section_text_safe_limit`) <> "\n… (truncated)" + else + text + end + endWhat is the current maximum character length for Slack Block Kit section text objects (mrkdwn/plain_text), and what error does chat.postMessage return when this limit is exceeded?Also applies to: 152-156
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/bike_brigade/slack_api/payload_builder.ex` around lines 123 - 148, build_rider_block and build_unassigned_block embed unbounded task_lines (from format_task_lines/1) into Slack section text which can exceed Slack's Block Kit limits and cause message rejection; fix by limiting the length of task_lines before embedding (either truncate with an ellipsis or split into additional section blocks) in both build_rider_block and build_unassigned_block, e.g., call a helper like safe_task_lines = truncate_task_lines(format_task_lines(tasks)) or split_task_lines(format_task_lines(unassigned_tasks)) and use safe_task_lines when building the mrkdwn text (ensure any truncation preserves filter_mrkdwn/escaping and that the helper enforces the Slack section text max length to avoid invalid_blocks).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/bike_brigade/campaign_summary_poster.ex`:
- Around line 79-82: send_to_slack/2 currently swallows
Slack.CampaignDeliverySummary.send_summary's result and always returns :ok;
update send_to_slack/2 to return the actual result from
Slack.CampaignDeliverySummary.send_summary (propagate {:ok, ...} or {:error,
reason} as-is). Then update send_and_mark_sent/3 to check send_to_slack/2's
return value and on a non-{:ok, _} result return {:error, reason} (and keep
logging) instead of returning {:ok, record}; ensure only when send_to_slack/2
returns {:ok, _} you mark the record sent and return {:ok, record}.
---
Duplicate comments:
In `@lib/bike_brigade/slack_api/payload_builder.ex`:
- Around line 123-148: build_rider_block and build_unassigned_block embed
unbounded task_lines (from format_task_lines/1) into Slack section text which
can exceed Slack's Block Kit limits and cause message rejection; fix by limiting
the length of task_lines before embedding (either truncate with an ellipsis or
split into additional section blocks) in both build_rider_block and
build_unassigned_block, e.g., call a helper like safe_task_lines =
truncate_task_lines(format_task_lines(tasks)) or
split_task_lines(format_task_lines(unassigned_tasks)) and use safe_task_lines
when building the mrkdwn text (ensure any truncation preserves
filter_mrkdwn/escaping and that the helper enforces the Slack section text max
length to avoid invalid_blocks).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9a5c5a7e-9e9c-427f-861a-9aa4175e0105
📒 Files selected for processing (5)
lib/bike_brigade/campaign_summary_poster.exlib/bike_brigade/delivery/campaign_delivery_summary.exlib/bike_brigade/messaging/slack.exlib/bike_brigade/messaging/slack_campaign_summary_message.exlib/bike_brigade/slack_api/payload_builder.ex
✅ Files skipped from review due to trivial changes (2)
- lib/bike_brigade/messaging/slack_campaign_summary_message.ex
- lib/bike_brigade/delivery/campaign_delivery_summary.ex
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/bike_brigade/messaging/slack.ex
Date calculations used Date.utc_today() and midnight UTC for campaign fixtures, causing two distinct failures: - Date.utc_today() returns Monday from midnight UTC (7-8 PM ET Sunday), so campaign datetimes were scheduled in Toronto's Sunday, not Monday, breaking the ISODOW day-of-week check in the monday filter. Switch to Toronto-timezone date arithmetic and noon Toronto time for campaign fixtures. Add safe_past_monday/1 to fall back to the previous week when noon Toronto on the target date hasn't passed yet — that Monday is at most 6d 23h ago, still safely within the 7-day window used by the week filter.
Replace the case-on-tuple pattern used to check date equality with a named boolean and an if/else. The new form states the intent directly.
Rename the nested Slack module from CampaignDeliverySummary to CampaignSummarySender to avoid naming confusion with the BikeBrigade.Delivery.CampaignDeliverySummary struct.
Extract channel validation logic into separate private functions for better code organization. Add Slack notification when a campaign's program lacks a configured channel, improving visibility of configuration issues.
- Refactor do_post_summary to use with statement for clearer flow - Consolidate find_or_create_record to handle existing record check - Add else clause to notify operations channel on errors - Inline prepare_summary and mark_as_sent functions
Large campaigns with many tasks can exceed Slack message size limits. When total tasks exceed the threshold, show a simplified summary with just the campaign link and delivery stats instead of per-rider details.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 13 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/bike_brigade/campaign_summary_poster.ex">
<violation number="1" location="lib/bike_brigade/campaign_summary_poster.ex:116">
P1: Slack send failures are swallowed, so campaign summaries can be permanently dropped after transient errors.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/bike_brigade/slack_api/payload_builder.ex">
<violation number="1" location="lib/bike_brigade/slack_api/payload_builder.ex:151">
P2: Keep the per-rider completion counts in the detailed summary; this change drops them from every rider block.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Align module and table naming for clarity: - Rename module from SlackCampaignSummaryMessage to SlackCampaignSummary - Rename table from slack_campaign_messages to slack_campaign_summaries - Rename schema file to slack_campaign_summary.ex - Update all references in CampaignSummaryPoster
Return {:error, :already_sent} instead of bare :already_sent atom to match the with clause pattern.
mveytsman
requested changes
Apr 17, 2026
Member
mveytsman
left a comment
mveytsman
left a comment
There was a problem hiding this comment.
Some design feedback otherwise it's good!
Move task fetching and summary building from CampaignSummaryPoster into the CampaignDeliverySummary module where it belongs.
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
2 participants
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.
Summary
Adds automated posting of campaign delivery summaries to Slack after campaigns end.
What This Does
A new GenServer (
CampaignSummaryPoster) runs every 15 minutes to find campaigns that ended 60-75 minutes ago and posts their delivery summaries to the program's configured Slack channel.Key Components
CampaignDeliverySummary
(
lib/bike_brigade/delivery/campaign_delivery_summary.ex)Collectableprotocol for use withEnum.into/2CampaignSummaryPoster
(
lib/bike_brigade/campaign_summary_poster.ex)SlackCampaignSummary
(
lib/bike_brigade/messaging/slack_campaign_summary.ex)campaign_idenables race-condition-free insertsSlack Block Kit Formatting
(
lib/bike_brigade/slack_api/payload_builder.ex)Changes
list_campaigns_ended_between/2query in Delivery contextslack_campaign_summariestable with unique constraint on campaign_idTest Coverage
Checklist before requesting a review
about this update in the description above.