New automations - #469
Merged
Merged
New automations#469
Conversation
# Conflicts: # i18n/da-DK.json # i18n/de-DE.json # i18n/es-ES.json # i18n/fa-IR.json # i18n/fr-FR.json # i18n/it-IT.json # i18n/ja-JP.json # i18n/mr-IN.json # i18n/pt-BR.json # internal/conversation/conversation.go
An automation rule that listens on an event and then writes the same field re-fires its own event, so the rule matches again and loops forever. Status already returned early on a no-op write, but priority and user assignment did not, so those two could spin a worker doing a DB write per lap. Add the same unchanged-value guard to both. Suppression while the engine applies actions was a plain flag in a sync.Map, so with more than one worker on the same conversation the first one to finish deleted the key while the other was still applying actions. Make it a refcount so each worker releases only its own claim. The starts with operator is only implemented by the automation evaluator, not the SQL filter builder, so give automation text fields their own operator list instead of adding it to the shared one. Also switch the notify action to its own recipients field type, fix the snooze duration hint since the backend only takes Go duration units, trim and lowercase notify recipient entries, and treat a missing previous value as no match rather than an empty string.
Contributor
|
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:
📝 WalkthroughWalkthroughThis change adds previous-value automation filters, starts-with matching, notification, snooze, and targeted webhook actions. It updates conversation event propagation, webhook delivery, frontend automation controls, translations, and evaluator tests. ChangesAutomation extensions
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Conversation
participant AutomationEngine
participant Evaluator
participant WebhookManager
participant NotificationDispatcher
Conversation->>AutomationEngine: submit event and previous values
AutomationEngine->>Evaluator: evaluate conversation rules
Evaluator->>Conversation: execute snooze action
Evaluator->>WebhookManager: trigger selected webhook
WebhookManager-->>Conversation: deliver webhook event
Conversation->>NotificationDispatcher: send resolved recipients notification
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
internal/automation/evaluator_test.go (1)
1454-1479: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the suppression refcount.
The new tests cover previous values, the starts-with operator, and action passthrough. They do not cover
suppress,unsuppress, orisSuppressed. That logic is the riskiest part of this change, because a leaked refcount silently disables automation for a conversation.Add a test that asserts
isSuppressedreturns true duringApplyActionand false afterevalConversationRulesreturns. A mockApplyActionthat callsengine.isSuppressed(conversation.UUID)makes this assertion direct.Also consider one case for
RuleOperatorStartsWithwithCaseSensitiveMatch: true, since only the default case-insensitive path is covered.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/automation/evaluator_test.go` around lines 1454 - 1479, Extend TestStartsWithOperator coverage with a mock ApplyAction that records engine.isSuppressed(conversation.UUID), then assert suppression is true during action execution and false after evalConversationRules returns. Also add a RuleOperatorStartsWith case using CaseSensitiveMatch: true to verify case-sensitive matching behavior.internal/automation/automation.go (1)
306-309: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftDo not drop non-automation update events behind per-conversation suppression.
EvaluateConversationUpdateRulesis called from conversation status, assignment, and priority writes; each of those paths is skipped while any automation worker holdsconversations/[uuid]. Those legitimate concurrent updates are lost instead of deferred.Restrict the suppression to automation-originated action context. Since
ApplyActionfalls back toumodels.User{}, the automation actor is identifiable at the update site.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/automation/automation.go` around lines 306 - 309, Update EvaluateConversationUpdateRules so per-conversation suppression applies only to automation-originated updates, not ordinary status, assignment, or priority writes. Use the actor context at the update site to identify automation actions, including the umodels.User{} fallback produced by ApplyAction, and bypass isSuppressed for non-automation actors.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/conversation/conversation.go`:
- Around line 1513-1524: Reject non-positive webhook IDs in the targeted
delivery validation around strconv.Atoi in internal/conversation/conversation.go
lines 1513-1524 by returning an error unless webhookID > 0. Add defense-in-depth
handling in internal/webhook/webhook.go lines 263-279 to reject or log and drop
non-positive IDs before delivery; both sites require changes, while preserving
valid targeted delivery.
In `@internal/conversation/models/models.go`:
- Around line 347-355: Update the automation path in ApplyAction for
ActionSendPrivateNote so the created private note includes metadata with
is_automated set to true, or extend SendPrivateNote to accept and apply that
flag. Ensure Message.IsAutomated() recognizes these notes so the existing
EvaluateConversationUpdateRulesByID gate excludes them.
---
Nitpick comments:
In `@internal/automation/automation.go`:
- Around line 306-309: Update EvaluateConversationUpdateRules so
per-conversation suppression applies only to automation-originated updates, not
ordinary status, assignment, or priority writes. Use the actor context at the
update site to identify automation actions, including the umodels.User{}
fallback produced by ApplyAction, and bypass isSuppressed for non-automation
actors.
In `@internal/automation/evaluator_test.go`:
- Around line 1454-1479: Extend TestStartsWithOperator coverage with a mock
ApplyAction that records engine.isSuppressed(conversation.UUID), then assert
suppression is true during action execution and false after
evalConversationRules returns. Also add a RuleOperatorStartsWith case using
CaseSensitiveMatch: true to verify case-sensitive matching behavior.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 40ded46c-9cfb-4e08-acae-1702e6b2f534
📒 Files selected for processing (13)
frontend/apps/main/src/composables/useConversationFilters.jsfrontend/apps/main/src/constants/filterConfig.jsfrontend/apps/main/src/features/admin/automation/ActionBox.vuefrontend/apps/main/src/stores/webhook.jsi18n/en-US.jsoninternal/automation/automation.gointernal/automation/evaluator.gointernal/automation/evaluator_test.gointernal/automation/models/models.gointernal/conversation/conversation.gointernal/conversation/message.gointernal/conversation/models/models.gointernal/webhook/webhook.go
strconv.Atoi accepts "0" and negative values, and the delivery worker treats a non-positive WebhookID as a fan out to every subscriber of the event. So a malformed target value sent the conversation payload to unintended webhooks. Reject it in the action and drop it in TriggerWebhook as well.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/webhook/webhook.go (1)
338-351: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSensitive Data Exposure (CWE-200): Exposure of Sensitive Information to an Unauthorized Actor
Reachability: External
Reachability path
● Entry cmd/webhooks.go:19 GetAllCompact │ ▼ ● Sink internal/webhook/webhook.goBlock redirect hops for signed webhook requests.
Manager.httpClienthas noCheckRedirect, so a307/308webhook response can resend the POST payload to another host. The signature is added manually indeliverSingleWebhook, so it is not automatically stripped across origins; a redirect policy should either disable redirects for these POSTs or validate every hop before allowing it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/webhook/webhook.go` around lines 338 - 351, Update the HTTP client used by deliverSingleWebhook to prevent signed webhook POST requests from following redirects, preferably by configuring Manager.httpClient with a CheckRedirect policy that returns http.ErrUseLastResponse or otherwise rejects redirect hops. Preserve normal delivery and error handling while ensuring 307/308 responses cannot resend the signed payload to another host.Source: MCP tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/webhook/webhook.go`:
- Around line 338-351: Update the HTTP client used by deliverSingleWebhook to
prevent signed webhook POST requests from following redirects, preferably by
configuring Manager.httpClient with a CheckRedirect policy that returns
http.ErrUseLastResponse or otherwise rejects redirect hops. Preserve normal
delivery and error handling while ensuring 307/308 responses cannot resend the
signed payload to another host.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 780fceae-d7a3-42f8-9c08-6dc60decbebe
📒 Files selected for processing (11)
cmd/handlers.gocmd/webhooks.gofrontend/apps/main/src/api/index.jsfrontend/apps/main/src/features/admin/automation/ActionBox.vuefrontend/apps/main/src/stores/webhook.jsi18n/en-US.jsoninternal/automation/automation.gointernal/conversation/conversation.gointernal/webhook/models/models.gointernal/webhook/queries.sqlinternal/webhook/webhook.go
💤 Files with no reviewable changes (1)
- internal/automation/automation.go
🚧 Files skipped from review as they are similar to previous changes (3)
- i18n/en-US.json
- frontend/apps/main/src/features/admin/automation/ActionBox.vue
- internal/conversation/conversation.go
The notify action only created a fixed in-app notification and recipients
were typed as a raw team:<id> / user:<id> DSL where typos silently notified
nobody. Now:
- recipients are picked by name (assignee, assigned team, any team or agent)
- admin writes the subject and message; both show in the bell notification
and go out as an email (message plus conversation link, wrapped in the
default outgoing email template, so nothing is hardcoded in English)
- the action serializes as typed fields {subject, message, recipients}
instead of a positional value array
Also: previous_* condition fields are hidden for time triggers and populated
on message events (previous = current) so those rules can actually match,
action row widths now follow the RuleBox pattern (fixed type select, value
widgets fill the row), and three new i18n keys merged into existing globals.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/conversation/message.go (1)
1391-1396: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCapture the previous conversation before hook updates.
ReOpenConversationruns before this fetch.PreviousValues(conversation)can therefore containstatus=openinstead of the prior status. An incoming-message rule cannot matchprevious_status=resolvedafter a customer reply reopens the conversation.Fetch a pre-hook snapshot before
UpdateConversationWaitingSinceandReOpenConversation. Passamodels.PreviousValues(previousConversation)at Line 1396. Add a regression test for a resolved conversation that receives an incoming message.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/conversation/message.go` around lines 1391 - 1396, Capture the conversation snapshot before UpdateConversationWaitingSince and ReOpenConversation execute, retain it through the incoming-message hook flow, and pass amodels.PreviousValues(previousConversation) to EvaluateConversationUpdateRules instead of deriving previous values from the post-reopen conversation. Add a regression test covering a resolved conversation receiving an incoming message and matching previous_status=resolved.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/conversation/message.go`:
- Around line 1391-1396: Capture the conversation snapshot before
UpdateConversationWaitingSince and ReOpenConversation execute, retain it through
the incoming-message hook flow, and pass
amodels.PreviousValues(previousConversation) to EvaluateConversationUpdateRules
instead of deriving previous values from the post-reopen conversation. Add a
regression test covering a resolved conversation receiving an incoming message
and matching previous_status=resolved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c75e6264-dc88-46f5-a849-73605d4fbd4a
📒 Files selected for processing (10)
frontend/apps/main/src/composables/useConversationFilters.jsfrontend/apps/main/src/features/admin/automation/ActionBox.vuefrontend/apps/main/src/features/admin/automation/RuleBox.vuefrontend/apps/main/src/views/admin/automations/CreateOrEditRule.vuei18n/en-US.jsoninternal/automation/automation.gointernal/automation/evaluator_test.gointernal/automation/models/models.gointernal/conversation/conversation.gointernal/conversation/message.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/automation/automation.go
- frontend/apps/main/src/composables/useConversationFilters.js
- internal/automation/evaluator_test.go
Automation update events now carry the actor. A suppressed conversation drops only system-user events (the engine's own in-flight actions), so an agent updating the conversation during that window is evaluated instead of silently dropped. Autoassigner events still trigger automations since they arrive outside the suppress window.
Owner
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/automation/automation.go`:
- Around line 333-340: Update EvaluateConversationUpdateRulesByID so it does not
call models.PreviousValues(conversation) on the post-update conversation; pass
nil when no pre-update snapshot is available, or extend the method to accept and
forward the actual pre-update values map when callers provide one. Preserve
conversation fetching and rule evaluation, and add a regression test covering
previous_status, previous_priority, or assignment filters through this method.
In `@internal/conversation/message.go`:
- Line 1396: Update the incoming-message flow around ReOpenConversation and
EvaluateConversationUpdateRules to load or retain the conversation snapshot
before reopening it, then pass that pre-reopen snapshot to
amodels.PreviousValues at the rule-evaluation call. Keep the reopened
conversation for current-state processing while ensuring previous values reflect
the state before reopening.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c1443f04-81e8-454b-82ac-17920df17bb7
📒 Files selected for processing (21)
cmd/handlers.gocmd/main.gocmd/webhooks.gofrontend/apps/main/src/api/index.jsfrontend/apps/main/src/composables/useConversationFilters.jsfrontend/apps/main/src/constants/filterConfig.jsfrontend/apps/main/src/features/admin/automation/ActionBox.vuefrontend/apps/main/src/features/admin/automation/RuleBox.vuefrontend/apps/main/src/stores/webhook.jsfrontend/apps/main/src/views/admin/automations/CreateOrEditRule.vuei18n/en-US.jsoninternal/automation/automation.gointernal/automation/evaluator.gointernal/automation/evaluator_test.gointernal/automation/models/models.gointernal/conversation/conversation.gointernal/conversation/message.gointernal/conversation/models/models.gointernal/webhook/models/models.gointernal/webhook/queries.sqlinternal/webhook/webhook.go
This was referenced Aug 12, 2026
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
1 participant
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 by CodeRabbit