feat: auto-disable webhooks after persistent failures#3587
Draft
feat: auto-disable webhooks after persistent failures#3587
Conversation
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds an Changes
Sequence DiagramsequenceDiagram
participant Scheduler as WebhookAutoDisableScheduler
participant Locking as LockingProvider
participant Service as WebhookConfigService
participant Repo as WebhookConfigRepository
participant Email as EmailService
participant DB as Database
Scheduler->>Locking: withLockingIfFree(lockName, leaseTime)
alt lock acquired
Scheduler->>Repo: findEnabledFailingNotNotified(warningCutoff)
Repo-->>Scheduler: webhooksToWarn
loop each webhook
Scheduler->>Service: markWarningNotified(webhook)
Service->>DB: set autoDisableNotified = true
Scheduler->>Email: send webhook-failing-warning to org owners
end
Scheduler->>Repo: findEnabledFailingSince(disableCutoff)
Repo-->>Scheduler: webhooksToDisable
loop each webhook
Scheduler->>Service: disableWebhook(webhook)
Service->>DB: set enabled = false
Scheduler->>Email: send webhook-disabled to org owners
end
else lock not acquired
Scheduler->>Scheduler: skip run
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 8
🧹 Nitpick comments (3)
backend/data/src/main/kotlin/io/tolgee/configuration/tolgee/TolgeeProperties.kt (1)
123-123: Optional: document the newwebhookconfig block in generated property docs.Line 123 works as-is, but adding
@DocPropertyhere would keep config docs as self-explanatory as the surrounding properties.📝 Suggested refinement
+ `@DocProperty`(description = "Webhook automation settings (e.g. auto-disable controls).") var webhook: WebhookProperties = WebhookProperties(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/data/src/main/kotlin/io/tolgee/configuration/tolgee/TolgeeProperties.kt` at line 123, Add the `@DocProperty` annotation to the TolgeeProperties.webhook property so the generated configuration docs include the new webhook config block; update the declaration for the webhook property (type WebhookProperties in class TolgeeProperties) to include `@DocProperty` with a brief description of the webhook config and any relevant default behavior so it appears alongside the other documented properties.e2e/cypress/e2e/projects/webhooks.cy.ts (1)
116-156: Reload after each toggle to verify persistence.This currently proves only that the local row state changed. A reload after disabling/enabling would catch cases where the UI updates optimistically but the PUT never persists
enabled, and it would let the test assert the re-fetched switch state instead of the immediate DOM styling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/cypress/e2e/projects/webhooks.cy.ts` around lines 116 - 156, After toggling the webhook in the test (using createWebhook(), item.findDcy('webhook-item-toggle').find('input').click() and waitForGlobalLoading()), reload the page and re-query the item via gcyAdvanced({ value: 'webhooks-list-item', url: testUrl }) before asserting persistence; do this both after disabling and after re-enabling so you assert the persisted switch state (should('not.be.checked') / should('be.checked')) and the persisted row opacity ('have.css','opacity','0.6' and '1') against the re-fetched DOM rather than the immediate optimistic DOM updates.backend/data/src/main/kotlin/io/tolgee/component/automations/processors/WebhookProcessor.kt (1)
29-32: Move the disabled guard before building the activity payload.Disabled webhooks still pay for
findProjectActivity()andtoModel()because theenabledcheck happens afterward. Readaction.webhookConfigand return on!enabledbefore Lines 29-30 so skipped hooks avoid the extra lookup and serialization.♻️ Suggested reorder
) { activityRevisionId ?: return + val config = action.webhookConfig ?: return + if (!config.enabled) return val view = activityService.findProjectActivity(activityRevisionId) ?: return val activityModel = activityModelAssembler.toModel(view) - val config = action.webhookConfig ?: return - if (!config.enabled) return val data =🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/data/src/main/kotlin/io/tolgee/component/automations/processors/WebhookProcessor.kt` around lines 29 - 32, In WebhookProcessor, avoid calling activityService.findProjectActivity(...) and activityModelAssembler.toModel(...) for disabled webhooks by reading action.webhookConfig and returning when config.enabled is false before you build the activity payload; move the guard that checks action.webhookConfig and config.enabled to execute prior to the calls to findProjectActivity(...) and toModel(...) so disabled hooks skip the lookup and serialization.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@backend/data/src/main/kotlin/io/tolgee/configuration/tolgee/WebhookProperties.kt`:
- Around line 6-33: Annotate the WebhookProperties class with Spring validation
(`@Validated`) and add bean-validation annotations to each numeric property (use
`@Positive` or `@Min`(1) for autoDisableCheckPeriodMs, autoDisableLockLeaseTimeMs
and `@Positive/`@Min(1) for autoDisableWarningAfterHours and autoDisableAfterDays)
so non-positive values fail startup; also add a `@PostConstruct` validation method
in WebhookProperties that checks autoDisableWarningAfterHours <
autoDisableAfterDays and throws an IllegalStateException with a clear message if
the ordering is invalid so misordered thresholds are rejected at startup.
In
`@ee/backend/app/src/main/kotlin/io/tolgee/ee/component/WebhookAutoDisableScheduler.kt`:
- Around line 63-68: In WebhookAutoDisableScheduler, don't persist the
autoDisableNotified/markWarningNotified flag before sending emails; instead call
webhookConfigService.markWarningNotified(webhook) only after sendEmails(webhook,
"webhook-failing-warning") succeeds and do that persistence outside the
executeInNewTransaction block to avoid including network I/O in the DB
transaction; also change sendEmails handling so failures surface (or return
per-recipient status) and only then update autoDisableNotified, and apply the
same change for the second occurrence (the block around lines 112-127) so
notifications are only marked when emails were actually dispatched.
In
`@ee/backend/tests/src/test/kotlin/io/tolgee/ee/component/WebhookAutoDisableSchedulerTest.kt`:
- Around line 142-155: The test does not verify that no second warning email was
sent; after calling webhookAutoDisableScheduler.checkAndDisable() in the test
does not send another email. Update the `does not send warning twice` test to
also assert the email/mock notification sender observed zero new sends for that
invocation (e.g., check the send method call count on your mail mock or
notification service mock) immediately after calling
`webhookAutoDisableScheduler.checkAndDisable()` and before reloading the config;
reference the test function name and the
`webhookAutoDisableScheduler.checkAndDisable()` call to locate where to add the
assertion.
In `@email/emails/webhook-disabled.tsx`:
- Line 41: The email copy in webhook-disabled.tsx currently hardcodes "more than
3 days" in the defaultValue string; replace that static text with a dynamic
value pulled from the app config or props (e.g., autoDisableDays or
autoDisableDuration) and interpolate it into the defaultValue alongside
{webhookUrl} and {projectName}; also ensure proper pluralization/formatting when
building the string (e.g., "after failing continuously for more than
{autoDisableDays} day(s)" or use a helper like formatDuration) so the message
reflects the actual configurable auto-disable threshold.
In `@email/emails/webhook-failing-warning.tsx`:
- Around line 11-14: The multiline JSX expression for the subject prop uses
t.raw across multiple lines which trips the template static-check; collapse the
call into a single-line expression by replacing the current multiline
subject={t.raw('webhook-failing-warning-subject', 'Webhook is failing')} block
with a single-line subject={t.raw('webhook-failing-warning-subject', 'Webhook is
failing')} so the t.raw invocation is flattened (locate the subject prop inside
the email component in webhook-failing-warning.tsx).
In `@email/i18n/messages_en.properties`:
- Around line 18-21: The email templates currently hardcode the auto-disable
timeout; update the keys webhook-disabled-body and webhook-failing-warning-body
to use a formatted placeholder (e.g., {autoDisableAfter} or {remainingTime})
instead of the literal "more than 3 days" and "{autoDisableAfterDays} days", and
ensure the placeholder name is consistent across templates so callers can inject
the configured duration (formatted as "3 days" or "48 hours" as appropriate);
keep webhook-disabled-action and webhook-failing-warning-subject unchanged and
update any code that sends these messages to supply the new placeholder value so
default copy remains consistent with configuration.
In `@webapp/src/ee/developer/webhook/WebhookItem.tsx`:
- Around line 101-110: The tooltip logic currently infers "Automatically
disabled…" using isAutoDisabled which only checks data.firstFailed; change it to
rely on an explicit backend flag (e.g. data.autoDisabled or data.disabledReason
=== 'auto') instead of data.firstFailed, and if that flag is absent use a
generic disabled tooltip; update the isAutoDisabled and toggleTooltip branches
(referencing isAutoDisabled, toggleTooltip, data.firstFailed, data.enabled) so
only a true explicit auto-disabled flag shows the "Automatically disabled…" text
and manual disables show a generic "Webhook disabled" message or the existing
enable/disable hints.
- Around line 120-126: The Switch control (the Switch component with props
checked={data.enabled}, onChange={handleToggle},
disabled={toggleWebhook.isLoading}, data-cy="webhook-item-toggle") is missing an
accessible name; add an aria-label or aria-labelledby that identifies the
webhook (for example including data.name or data.id) or describes the action
("Enable webhook <name>" / "Disable webhook <name>") so screen readers announce
the toggle; ensure the label updates or is dynamic based on data.enabled and use
aria-labelledby if there's an existing visible element ID that names the
webhook.
---
Nitpick comments:
In
`@backend/data/src/main/kotlin/io/tolgee/component/automations/processors/WebhookProcessor.kt`:
- Around line 29-32: In WebhookProcessor, avoid calling
activityService.findProjectActivity(...) and activityModelAssembler.toModel(...)
for disabled webhooks by reading action.webhookConfig and returning when
config.enabled is false before you build the activity payload; move the guard
that checks action.webhookConfig and config.enabled to execute prior to the
calls to findProjectActivity(...) and toModel(...) so disabled hooks skip the
lookup and serialization.
In
`@backend/data/src/main/kotlin/io/tolgee/configuration/tolgee/TolgeeProperties.kt`:
- Line 123: Add the `@DocProperty` annotation to the TolgeeProperties.webhook
property so the generated configuration docs include the new webhook config
block; update the declaration for the webhook property (type WebhookProperties
in class TolgeeProperties) to include `@DocProperty` with a brief description of
the webhook config and any relevant default behavior so it appears alongside the
other documented properties.
In `@e2e/cypress/e2e/projects/webhooks.cy.ts`:
- Around line 116-156: After toggling the webhook in the test (using
createWebhook(), item.findDcy('webhook-item-toggle').find('input').click() and
waitForGlobalLoading()), reload the page and re-query the item via gcyAdvanced({
value: 'webhooks-list-item', url: testUrl }) before asserting persistence; do
this both after disabling and after re-enabling so you assert the persisted
switch state (should('not.be.checked') / should('be.checked')) and the persisted
row opacity ('have.css','opacity','0.6' and '1') against the re-fetched DOM
rather than the immediate optimistic DOM updates.
🪄 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: 2c4a6e33-043e-4a78-89a5-ea381300f90a
📒 Files selected for processing (21)
backend/api/src/main/kotlin/io/tolgee/hateoas/ee/webhooks/WebhookConfigModel.ktbackend/app/src/test/resources/application.yamlbackend/data/src/main/kotlin/io/tolgee/component/automations/processors/WebhookProcessor.ktbackend/data/src/main/kotlin/io/tolgee/configuration/tolgee/TolgeeProperties.ktbackend/data/src/main/kotlin/io/tolgee/configuration/tolgee/WebhookProperties.ktbackend/data/src/main/kotlin/io/tolgee/dtos/request/WebhookConfigRequest.ktbackend/data/src/main/kotlin/io/tolgee/model/webhook/WebhookConfig.ktbackend/data/src/main/kotlin/io/tolgee/repository/WebhookConfigRepository.ktbackend/data/src/main/resources/db/changelog/schema.xmle2e/cypress/e2e/projects/webhooks.cy.tsee/backend/app/src/main/kotlin/io/tolgee/ee/api/v2/hateoas/assemblers/WebhookConfigModelAssembler.ktee/backend/app/src/main/kotlin/io/tolgee/ee/component/WebhookAutoDisableScheduler.ktee/backend/app/src/main/kotlin/io/tolgee/ee/service/WebhookConfigService.ktee/backend/tests/src/test/kotlin/io/tolgee/ee/WebhookAutomationTest.ktee/backend/tests/src/test/kotlin/io/tolgee/ee/api/v2/controllers/WebhookConfigControllerTest.ktee/backend/tests/src/test/kotlin/io/tolgee/ee/component/WebhookAutoDisableSchedulerTest.ktemail/emails/webhook-disabled.tsxemail/emails/webhook-failing-warning.tsxemail/i18n/messages_en.propertieswebapp/src/ee/developer/webhook/WebhookItem.tsxwebapp/src/service/apiSchema.generated.ts
Comment on lines
+6
to
+33
| @ConfigurationProperties(prefix = "tolgee.webhook") | ||
| @DocProperty(description = "Configuration for webhook behavior.", displayName = "Webhooks") | ||
| class WebhookProperties { | ||
| @DocProperty( | ||
| description = "Whether the automatic disabling of failing webhooks is enabled.", | ||
| ) | ||
| var autoDisableEnabled: Boolean = true | ||
|
|
||
| @DocProperty( | ||
| description = "How often (in milliseconds) to check for webhooks to auto-disable.", | ||
| ) | ||
| var autoDisableCheckPeriodMs: Long = 3_600_000 | ||
|
|
||
| @DocProperty( | ||
| description = "Number of hours of continuous failure before a warning email is sent.", | ||
| ) | ||
| var autoDisableWarningAfterHours: Int = 6 | ||
|
|
||
| @DocProperty( | ||
| description = "Number of days of continuous failure before a webhook is automatically disabled.", | ||
| ) | ||
| var autoDisableAfterDays: Int = 3 | ||
|
|
||
| @DocProperty( | ||
| description = "Lease time (in milliseconds) for the distributed lock used by the auto-disable job.", | ||
| ) | ||
| var autoDisableLockLeaseTimeMs: Long = 600_000 | ||
| } |
Contributor
There was a problem hiding this comment.
Fail fast on invalid webhook auto-disable settings.
These values are trusted as-is. Non-positive periods/lease times can break scheduling, and a misordered warning/disable threshold can make webhooks eligible for disable before any warning is ever valid. Please add bean validation so bad config is rejected at startup.
Suggested hardening
+import jakarta.validation.constraints.AssertTrue
+import jakarta.validation.constraints.Min
+import jakarta.validation.constraints.Positive
import org.springframework.boot.context.properties.ConfigurationProperties
+import org.springframework.validation.annotation.Validated
`@ConfigurationProperties`(prefix = "tolgee.webhook")
+@Validated
`@DocProperty`(description = "Configuration for webhook behavior.", displayName = "Webhooks")
class WebhookProperties {
@@
+ `@field`:Positive
var autoDisableCheckPeriodMs: Long = 3_600_000
@@
+ `@field`:Min(1)
var autoDisableWarningAfterHours: Int = 6
@@
+ `@field`:Min(1)
var autoDisableAfterDays: Int = 3
@@
+ `@field`:Positive
var autoDisableLockLeaseTimeMs: Long = 600_000
+
+ `@get`:AssertTrue(
+ message =
+ "tolgee.webhook.autoDisableWarningAfterHours must be lower than autoDisableAfterDays * 24",
+ )
+ val isThresholdOrderValid: Boolean
+ get() = autoDisableWarningAfterHours < autoDisableAfterDays * 24
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@backend/data/src/main/kotlin/io/tolgee/configuration/tolgee/WebhookProperties.kt`
around lines 6 - 33, Annotate the WebhookProperties class with Spring validation
(`@Validated`) and add bean-validation annotations to each numeric property (use
`@Positive` or `@Min`(1) for autoDisableCheckPeriodMs, autoDisableLockLeaseTimeMs
and `@Positive/`@Min(1) for autoDisableWarningAfterHours and autoDisableAfterDays)
so non-positive values fail startup; also add a `@PostConstruct` validation method
in WebhookProperties that checks autoDisableWarningAfterHours <
autoDisableAfterDays and throws an IllegalStateException with a clear message if
the ordering is invalid so misordered thresholds are rejected at startup.
Comment on lines
+63
to
+68
| executeInNewTransaction(transactionManager) { | ||
| val webhooks = webhookConfigService.findWebhooksToWarn(warningCutoff) | ||
|
|
||
| webhooks.forEach { webhook -> | ||
| webhookConfigService.markWarningNotified(webhook) | ||
| sendEmails(webhook, "webhook-failing-warning") |
Contributor
There was a problem hiding this comment.
Persist autoDisableNotified only after the warning email is actually sent.
Right now the flag is written before the email call, and sendEmails() swallows per-recipient failures. A transient mail issue will therefore suppress every later warning attempt even though no owner was notified. Move the state update behind a successful dispatch, ideally outside the transaction that performs the network I/O.
Also applies to: 112-127
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@ee/backend/app/src/main/kotlin/io/tolgee/ee/component/WebhookAutoDisableScheduler.kt`
around lines 63 - 68, In WebhookAutoDisableScheduler, don't persist the
autoDisableNotified/markWarningNotified flag before sending emails; instead call
webhookConfigService.markWarningNotified(webhook) only after sendEmails(webhook,
"webhook-failing-warning") succeeds and do that persistence outside the
executeInNewTransaction block to avoid including network I/O in the DB
transaction; also change sendEmails handling so failures surface (or return
per-recipient status) and only then update autoDisableNotified, and apply the
same change for the second occurrence (the block around lines 112-127) so
notifications are only marked when emails were actually dispatched.
Comment on lines
+142
to
+155
| @Test | ||
| fun `does not send warning twice`() { | ||
| testData.webhookConfig.self.firstFailed = currentDateProvider.date.addMinutes(-420) | ||
| testData.webhookConfig.self.enabled = true | ||
| testData.webhookConfig.self.autoDisableNotified = true | ||
| testDataService.saveTestData(testData.root) | ||
|
|
||
| webhookAutoDisableScheduler.checkAndDisable() | ||
|
|
||
| // Should still be notified, but no new email | ||
| val config = webhookConfigService.get(testData.webhookConfig.self.id) | ||
| config.autoDisableNotified.assert.isTrue | ||
| config.enabled.assert.isTrue | ||
| } |
Contributor
There was a problem hiding this comment.
This doesn't prove a second warning email wasn't sent.
The assertions only verify persisted flags, so the test still passes if the scheduler sends another warning. Please also assert that the email mock observed zero new sends in this path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@ee/backend/tests/src/test/kotlin/io/tolgee/ee/component/WebhookAutoDisableSchedulerTest.kt`
around lines 142 - 155, The test does not verify that no second warning email
was sent; after calling webhookAutoDisableScheduler.checkAndDisable() in the
test does not send another email. Update the `does not send warning twice` test
to also assert the email/mock notification sender observed zero new sends for
that invocation (e.g., check the send method call count on your mail mock or
notification service mock) immediately after calling
`webhookAutoDisableScheduler.checkAndDisable()` and before reloading the config;
reference the test function name and the
`webhookAutoDisableScheduler.checkAndDisable()` call to locate where to add the
assertion.
| <Text> | ||
| <LocalizedText | ||
| keyName="webhook-disabled-body" | ||
| defaultValue="The webhook {webhookUrl} in project {projectName} has been automatically disabled after failing continuously for more than 3 days." |
Contributor
There was a problem hiding this comment.
Avoid hardcoded disable duration in email copy.
Line 41 hardcodes “more than 3 days”, but auto-disable threshold is configurable. This can send incorrect information on customized instances.
✏️ Proposed text fix (config-agnostic)
- defaultValue="The webhook {webhookUrl} in project {projectName} has been automatically disabled after failing continuously for more than 3 days."
+ defaultValue="The webhook {webhookUrl} in project {projectName} has been automatically disabled after failing continuously for an extended period."📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| defaultValue="The webhook {webhookUrl} in project {projectName} has been automatically disabled after failing continuously for more than 3 days." | |
| defaultValue="The webhook {webhookUrl} in project {projectName} has been automatically disabled after failing continuously for an extended period." |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@email/emails/webhook-disabled.tsx` at line 41, The email copy in
webhook-disabled.tsx currently hardcodes "more than 3 days" in the defaultValue
string; replace that static text with a dynamic value pulled from the app config
or props (e.g., autoDisableDays or autoDisableDuration) and interpolate it into
the defaultValue alongside {webhookUrl} and {projectName}; also ensure proper
pluralization/formatting when building the string (e.g., "after failing
continuously for more than {autoDisableDays} day(s)" or use a helper like
formatDuration) so the message reflects the actual configurable auto-disable
threshold.
Comment on lines
+18
to
+21
| webhook-disabled-body = The webhook {webhookUrl} in project {projectName} has been automatically disabled after failing continuously for more than 3 days. | ||
| webhook-disabled-action = You can re-enable it from the project's Developer settings in the Webhooks section. | ||
| webhook-failing-warning-subject = Webhook is failing | ||
| webhook-failing-warning-body = The webhook {webhookUrl} in project {projectName} has been failing continuously. If the issue is not resolved, the webhook will be automatically disabled in {autoDisableAfterDays} days. |
Contributor
There was a problem hiding this comment.
Don't hardcode the auto-disable timeout in these strings.
The warning/disable thresholds are configurable, so "more than 3 days" and "{autoDisableAfterDays} days" become misleading as soon as an instance uses anything other than the default. The warning email also is not necessarily sent with 3 days remaining. Please switch these to a formatted duration or remaining-time placeholder and keep the template default copy aligned.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@email/i18n/messages_en.properties` around lines 18 - 21, The email templates
currently hardcode the auto-disable timeout; update the keys
webhook-disabled-body and webhook-failing-warning-body to use a formatted
placeholder (e.g., {autoDisableAfter} or {remainingTime}) instead of the literal
"more than 3 days" and "{autoDisableAfterDays} days", and ensure the placeholder
name is consistent across templates so callers can inject the configured
duration (formatted as "3 days" or "48 hours" as appropriate); keep
webhook-disabled-action and webhook-failing-warning-subject unchanged and update
any code that sends these messages to supply the new placeholder value so
default copy remains consistent with configuration.
Comment on lines
+101
to
+110
| const isAutoDisabled = !data.enabled && data.firstFailed; | ||
|
|
||
| const toggleTooltip = isAutoDisabled | ||
| ? t( | ||
| 'webhook_auto_disabled_hint', | ||
| 'Automatically disabled due to persistent failures' | ||
| ) | ||
| : data.enabled | ||
| ? t('webhook_toggle_disable_hint', 'Disable webhook') | ||
| : t('webhook_toggle_enable_hint', 'Enable webhook'); |
Contributor
There was a problem hiding this comment.
Don't infer the disable reason from firstFailed alone.
A webhook that a user disables manually while it is already failing will still satisfy this condition and get the “Automatically disabled…” tooltip. This needs an explicit backend reason flag, or the disabled branch should fall back to a generic tooltip instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@webapp/src/ee/developer/webhook/WebhookItem.tsx` around lines 101 - 110, The
tooltip logic currently infers "Automatically disabled…" using isAutoDisabled
which only checks data.firstFailed; change it to rely on an explicit backend
flag (e.g. data.autoDisabled or data.disabledReason === 'auto') instead of
data.firstFailed, and if that flag is absent use a generic disabled tooltip;
update the isAutoDisabled and toggleTooltip branches (referencing
isAutoDisabled, toggleTooltip, data.firstFailed, data.enabled) so only a true
explicit auto-disabled flag shows the "Automatically disabled…" text and manual
disables show a generic "Webhook disabled" message or the existing
enable/disable hints.
Comment on lines
+120
to
+126
| <Switch | ||
| size="small" | ||
| checked={data.enabled} | ||
| onChange={handleToggle} | ||
| disabled={toggleWebhook.isLoading} | ||
| data-cy="webhook-item-toggle" | ||
| /> |
Contributor
There was a problem hiding this comment.
Add an accessible name to the toggle.
The tooltip text does not label the control for assistive tech, so this will be announced as an unlabeled switch/button. Please attach an aria-label/aria-labelledby that identifies the webhook or the enable/disable action.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@webapp/src/ee/developer/webhook/WebhookItem.tsx` around lines 120 - 126, The
Switch control (the Switch component with props checked={data.enabled},
onChange={handleToggle}, disabled={toggleWebhook.isLoading},
data-cy="webhook-item-toggle") is missing an accessible name; add an aria-label
or aria-labelledby that identifies the webhook (for example including data.name
or data.id) or describes the action ("Enable webhook <name>" / "Disable webhook
<name>") so screen readers announce the toggle; ensure the label updates or is
dynamic based on data.enabled and use aria-labelledby if there's an existing
visible element ID that names the webhook.
Add webhook enabled/disabled toggle with automatic disabling when a webhook fails continuously for 3 days. Organization owners are notified via email at two stages: a warning after 6 hours of failure, and a notification when the webhook is disabled. Changes: - Add `enabled` and `autoDisableNotified` fields to WebhookConfig - Fix firstFailed bug (was overwritten on every failure instead of only on first failure) - Skip disabled webhooks during processing - Add scheduled job using SchedulingManager + LockingProvider pattern - Add React Email templates for warning and disable notifications - Add UI toggle switch in webhook list - Add backend integration tests and E2E tests Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Validate webhook properties at startup (warning hours < disable days) - Send warning email before marking autoDisableNotified (not after) - Use configurable autoDisableAfterDays in disabled email template - Assert no email sent in "does not send warning twice" test Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
JanCizmar
force-pushed
the
jancizmar/webhook-auto-disable
branch
from
eb4a9e7 to
b9f065f
Compare
- Add confirmation dialog when disabling a webhook (no prompt on enable) - Add defaultExplanation to WebhookProperties time-based fields - Expose internal /webhook-auto-disable/run endpoint for E2E testing - Refactor webhooks E2E tests to use compound objects (E2WebhooksView, E2WebhookItem, E2WebhookEditDialog) - Add E2E test verifying the full auto-disable flow: set firstFailed via SQL, trigger scheduler, check webhook disabled + email sent
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
firstFailedwas overwritten on every failure instead of tracking the initial failure timestampChanges
Backend
enabledandautoDisableNotifiedfields toWebhookConfigentity with DB migrationsfirstFailedtracking inWebhookProcessor— only set on first failure, clear on successWebhookAutoDisableSchedulerusingSchedulingManager+LockingProviderpattern (same asKeyTrashPurgeScheduler)WebhookPropertiesfor configurable thresholds (warning after 6h, disable after 3d)Email
webhook-disabled.tsxReact Email templatewebhook-failing-warning.tsxReact Email templateFrontend
Switchtoggle toWebhookItemwith disabled state (dimmed row, tooltip)enabledfieldTests
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Documentation