feat: allow http trigger to be disabled #6976

dieriba · 2025-10-28T12:08:05Z

No description provided.

cloudflare-workers-and-pages · 2025-10-28T12:10:18Z

Deploying windmill with Cloudflare Pages

Latest commit:	`7f4a9d1`
Status:	✅ Deploy successful!
Preview URL:	https://56f0657e.windmill.pages.dev
Branch Preview URL:	https://dieri-allow-http-trigger-to.windmill.pages.dev

View logs

claude · 2025-11-05T16:30:28Z

Claude Code is working…

I'll analyze this and get back to you.

View job run

backend/windmill-api/openapi.yaml

Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>

claude · 2025-11-06T11:55:37Z

Claude finished @dieriba's task —— View job

PR Review: Allow HTTP Trigger to be Disabled

I've completed a comprehensive review of this PR. The feature implementation is solid overall, but there are several important issues that should be addressed before merging.

Summary

This PR adds the ability to enable/disable HTTP triggers in Windmill. The implementation includes:

Database migration to add an enabled column to the http_trigger table
Backend API endpoint to toggle the enabled state
Frontend UI with a toggle switch in the routes list
Check in the request handler to reject requests to disabled triggers

Critical Issues 🔴

1. Migration Missing Default Value (backend/migrations/...up.sql:2)

The migration adds enabled BOOLEAN without a default value or NOT NULL constraint. This causes:

All existing triggers will have enabled = NULL after migration
The runtime check uses .unwrap_or(false), making all existing triggers disabled by default
This is a breaking change that will disable all production routes

Fix Required:

ALTER TABLE http_trigger ADD COLUMN enabled BOOLEAN NOT NULL DEFAULT true;

2. Inconsistent Default Behavior (handler.rs:860 + RouteEditorInner.svelte:273)

There are conflicting defaults throughout the codebase:

New trigger insertion: defaults to true ✓
Route execution check: defaults to false ✗
Frontend load: defaults to false ✗

This creates confusing behavior where new triggers work but existing ones don't.

Fix Required: Ensure consistent true defaults everywhere, or fix the migration to set existing rows to true.

Major Issues 🟡

3. Non-Admin Users Can't Update Enabled State (handler.rs:514)

The non-admin update path (lines 535-586) doesn't include the enabled field, while the admin path does. This means users with write permissions cannot enable/disable triggers they own.

Question: Is this intentional? If so, please document. Otherwise, add enabled to the non-admin UPDATE.

4. Filename Typo (migration files)

Migration files are named allow_to_diasable_http_trigger instead of allow_to_disable_http_trigger.

Performance Considerations 🔵

5. Disabled Triggers Loaded Into Router Cache (mod.rs:248)

The refresh_routers function loads ALL triggers regardless of enabled state. The check only happens at request time. Consider:

Filtering disabled triggers at load time to reduce memory footprint
OR documenting this design choice (if you want faster enable/disable without cache refresh)

Minor Issues & Suggestions

6. OpenAPI Spec Typo (Already noted by ellipsis-dev)

Line 9909: "enable/disalbe" → "enable/disable"

7. Missing Loading State in UI (routes/+page.svelte:362)

The toggle switch doesn't show loading state while the API call is in progress. Consider adding visual feedback.

8. Good Practices Observed ✅

Proper permission checks (canWrite)
API endpoint correctly updates metadata (email, edited_by, edited_at)
Frontend correctly passes enabled in save operations
Deployment metadata is handled properly

Security Assessment

No security issues identified. The feature properly:

Checks user permissions before allowing enable/disable
Uses existing authentication and authorization patterns
Includes audit logging via deployment metadata

Code Quality

The code follows Windmill's established patterns well:

✅ Uses the TriggerCrud trait pattern consistently
✅ Includes both frontend and backend changes
✅ Follows Rust best practices (Result types, proper error handling)
✅ Uses Svelte 5 patterns correctly ($state, $effect, etc.)
⚠️ Could benefit from more consistent null handling

Testing Recommendations

Before merging, please test:

Migration on database with existing triggers - Verify they remain enabled
Enable/disable toggle in UI - Should work smoothly with visual feedback
Disabled trigger behavior - Should return appropriate error message
Non-admin user permissions - Verify they can/cannot toggle based on design intent
Router cache refresh - Ensure changes take effect within 60 seconds

Reproduction Instructions for Testing

To test the changes in the Windmill application interface:

Navigate to the Routes page (accessible from the main navigation menu under "Triggers" or directly at /routes). In the routes list, you'll see a toggle switch next to each HTTP route entry. Click the toggle to enable or disable a route - when disabled, the toggle should show an "off" state and any HTTP requests to that route will return a 400 error stating the trigger is disabled. Users without write permissions will see the toggle but it will be disabled (not clickable). Admins can enable/disable any route, while non-admin users can only toggle routes they have write access to. The route's enabled/disabled state persists across page refreshes and is immediately effective (within 60 seconds due to router cache refresh). You can verify the behavior by attempting to call a disabled route's URL - it should reject the request with an error message indicating the trigger is disabled.

Recommendation

Request Changes - The migration issue (#1) is critical and will cause breaking changes in production. Please address the critical issues before merging. The other issues are important for code quality and UX but not blockers.

claude · 2025-11-06T11:57:27Z

backend/migrations/20251028111213_allow_to_diasable_http_trigger.up.sql

@@ -0,0 +1,2 @@
+-- Add up migration script here
+ALTER TABLE http_trigger ADD COLUMN enabled BOOLEAN;


Migration Issue: Missing default value

The enabled column is added without a NOT NULL constraint or a default value. This could lead to:

Existing triggers having NULL values - All existing HTTP triggers will have enabled = NULL after this migration runs

Logic issues - The code checks trigger.enabled.unwrap_or(false) (line 860 in handler.rs), which means existing triggers will default to false (disabled) after migration

Recommendation:

ALTER TABLE http_trigger ADD COLUMN enabled BOOLEAN NOT NULL DEFAULT true;

This ensures:

Existing HTTP triggers remain enabled by default (backward compatible)

New triggers are enabled by default (unless explicitly set otherwise)

No NULL handling needed in application code

claude · 2025-11-06T11:57:28Z

backend/migrations/20251028111213_allow_to_diasable_http_trigger.up.sql

@@ -0,0 +1,2 @@
+-- Add up migration script here


Typo in filename: The migration file is named allow_to_diasable_http_trigger but should be allow_to_disable_http_trigger (typo: "diasable" → "disable")

claude · 2025-11-06T11:57:29Z

backend/windmill-api/src/triggers/http/handler.rs

    .await
    .map_err(|e| e.into_response())?;

+    if !trigger.enabled.unwrap_or(false) {


Potential Issue: Inconsistent behavior with NULL enabled field

The check trigger.enabled.unwrap_or(false) defaults to false when enabled is NULL. However:

In insert_new_trigger_into_db (line 228), new triggers use trigger.base.enabled.unwrap_or(true) - defaulting to true

This creates inconsistent behavior where:

New triggers are enabled by default ✓

Existing triggers (after migration) will be disabled by default ✗

Recommendation:
Either:

Change this to .unwrap_or(true) to match insertion behavior, OR

Ensure the migration adds the column with DEFAULT true (see migration comment)

claude · 2025-11-06T11:57:30Z

backend/windmill-api/src/triggers/http/mod.rs

                        script_path,
                        is_flow,
                        route_path,
+                        enabled,


Query Performance Concern

This query in refresh_routers loads ALL HTTP triggers regardless of their enabled state. This means:

Disabled triggers are still loaded into the router cache

The check happens later at request time (line 860 in handler.rs)

This adds memory overhead and potentially slower router matching

Recommendation: Consider filtering disabled triggers at load time:

SELECT ... FROM http_trigger WHERE http_method = $1 AND (enabled IS NULL OR enabled = true)

This would:

Reduce memory footprint

Improve router matching performance

Prevent disabled routes from being accessible at all

Note: If you want to keep disabled routes in the router for faster enable/disable (avoiding cache refresh), the current approach is acceptable but should be documented.

you should filter out disabled routes

claude · 2025-11-06T11:57:32Z

backend/windmill-api/src/triggers/http/handler.rs

                trigger.base.script_path,
                trigger.base.path,
                trigger.base.is_flow,
+                trigger.base.enabled,


Missing enabled field in non-admin update path

When a non-admin user updates a trigger (lines 535-586), the enabled field is not included in the UPDATE statement. This means:

Non-admin users cannot modify the enabled state even if they have write permissions

The enabled value in trigger.base.enabled is silently ignored

Question: Is this intentional? If so, it should be documented. If not, consider adding enabled = $N to the non-admin UPDATE statement as well.

The admin path (lines 473-531) correctly includes enabled = $10.

claude · 2025-11-06T11:57:33Z

frontend/src/lib/components/triggers/http/RouteEditorInner.svelte

 		wrap_body = cfg?.wrap_body ?? false
 		raw_string = cfg?.raw_string ?? false
 		summary = cfg?.summary ?? ''
+		enabled = cfg?.enabled ?? false


Inconsistent default value

When loading trigger config, enabled defaults to false:

enabled = cfg?.enabled ?? false

However:

In openNew() (line 239), enabled defaults to true

In the backend insertion (handler.rs:228), it defaults to true

The backend check (handler.rs:860) defaults to false for NULL values

Recommendation: Change this line to:

enabled = cfg?.enabled ?? true

This ensures consistency across the codebase and better user experience (routes enabled by default).

that will make existing triggers shown as disabled

claude · 2025-11-06T11:57:34Z

frontend/src/lib/components/triggers/http/utils.ts

 		error_handler_args: routeCfg.error_handler_path ? routeCfg.error_handler_args : undefined,
-		retry: routeCfg.retry
+		retry: routeCfg.retry,
+		enabled: routeCfg.enabled


New field added to save function

Good! The enabled field is correctly included in the request body for both create and update operations.

claude · 2025-11-06T11:57:35Z

frontend/src/routes/(root)/(logged)/routes/+page.svelte

 									<SharedBadge {canWrite} extraPerms={extra_perms} />
 								</div>

+								<Toggle


UI Implementation looks good

The Toggle component is:

Correctly bound to the enabled state

Disabled when user lacks write permissions (disabled={!canWrite})

Calls setTriggerEnabled on change

One minor suggestion: Consider adding visual feedback (loading state) while the API call is in progress, since setTriggerEnabled is async but there's no loading indicator shown to the user.

hugocasa

you either have to set default to true in the migration or handle enabled is null correctly everywhere

hugocasa · 2025-11-06T14:52:32Z

backend/windmill-api/src/triggers/http/handler.rs

                trigger.base.script_path,
                trigger.base.path,
                trigger.base.is_flow,
+                trigger.base.enabled,


hugocasa · 2025-11-06T14:52:44Z

backend/windmill-api/src/triggers/http/handler.rs

    .await
    .map_err(|e| e.into_response())?;

+    if !trigger.enabled.unwrap_or(false) {


hugocasa · 2025-11-06T14:53:14Z

backend/windmill-api/src/triggers/http/mod.rs

                        script_path,
                        is_flow,
                        route_path,
+                        enabled,


you should filter out disabled routes

hugocasa · 2025-11-06T14:54:43Z

backend/windmill-api/src/triggers/listener.rs

+            "enabled",
+            "error_handler_path",
+            "error_handler_args",
+            "retry",


is it safe because this fn is only called when supports_server_state is set to true?

yup its because all listener object have supports_server_state to true thats why its safe

hugocasa · 2025-11-06T14:55:16Z

frontend/src/lib/components/triggers/http/RouteEditorInner.svelte

 		wrap_body = cfg?.wrap_body ?? false
 		raw_string = cfg?.raw_string ?? false
 		summary = cfg?.summary ?? ''
+		enabled = cfg?.enabled ?? false


that will make existing triggers shown as disabled

hugocasa · 2025-11-07T13:06:34Z

backend/windmill-api/src/triggers/http/mod.rs

                        script_path,
                        is_flow,
                        route_path,
+                        enabled,


you don't need enabled anymore as you know it will be enabled

hugocasa · 2025-11-07T13:07:34Z

backend/windmill-api/src/triggers/listener.rs

+            "enabled",
+            "error_handler_path",
+            "error_handler_args",
+            "retry",


is it safe because this fn is only called when supports_server_state is set to true?

hugocasa · 2025-11-07T13:09:48Z

backend/windmill-api/src/triggers/http/handler.rs

        Ok(())
    }

+    async fn set_enabled(


isn't already handled by the trait?

it is but for http we need to increase_trigger_version otherwise the cache we'd have outdated http trigger

done

428d4ca

dieriba added 5 commits October 28, 2025 14:29

Merge branch 'main' into dieri/allow-http-trigger-to-be-disabled

782b24f

update sqlx

cfaa6db

Merge branch 'main' into dieri/allow-http-trigger-to-be-disabled

82bcc36

fix

870c01d

fix openapi spec

150004a

dieriba marked this pull request as ready for review November 5, 2025 16:30

dieriba requested review from alpetric, hugocasa and rubenfiszel as code owners November 5, 2025 16:30

ellipsis-dev bot reviewed Nov 5, 2025

View reviewed changes

backend/windmill-api/openapi.yaml Outdated Show resolved Hide resolved

dieriba marked this pull request as draft November 5, 2025 16:36

dieriba and others added 6 commits November 5, 2025 17:37

fix

6405e6e

Update backend/windmill-api/openapi.yaml

ed5a514

Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>

nits

5075c36

Merge branch 'main' into dieri/allow-http-trigger-to-be-disabled

29c4fb4

nits

b1b61ee

fix

83bae7d

dieriba marked this pull request as ready for review November 6, 2025 11:55

claude bot reviewed Nov 6, 2025

View reviewed changes

hugocasa reviewed Nov 6, 2025

View reviewed changes

dieriba added 2 commits November 6, 2025 18:33

fix

5d84480

update sqlx

51d26de

hugocasa reviewed Nov 7, 2025

View reviewed changes

dieriba added 5 commits November 7, 2025 14:55

unneeded

a783799

Merge branch 'main' into dieri/allow-http-trigger-to-be-disabled

2744e34

ok

6083477

Merge branch 'main' into dieri/allow-http-trigger-to-be-disabled

750aa16

update sqlx

7f4a9d1

		@@ -0,0 +1,2 @@
		-- Add up migration script here
		ALTER TABLE http_trigger ADD COLUMN enabled BOOLEAN; No newline at end of file

feat: allow http trigger to be disabled #6976

Are you sure you want to change the base?

feat: allow http trigger to be disabled #6976

Uh oh!

Conversation

dieriba commented Oct 28, 2025

Uh oh!

cloudflare-workers-and-pages bot commented Oct 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Deploying windmill with Cloudflare Pages

Uh oh!

claude bot commented Nov 5, 2025

Uh oh!

Uh oh!

claude bot commented Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review: Allow HTTP Trigger to be Disabled

Summary

Critical Issues 🔴

1. Migration Missing Default Value (backend/migrations/...up.sql:2)

2. Inconsistent Default Behavior (handler.rs:860 + RouteEditorInner.svelte:273)

Major Issues 🟡

3. Non-Admin Users Can't Update Enabled State (handler.rs:514)

4. Filename Typo (migration files)

Performance Considerations 🔵

5. Disabled Triggers Loaded Into Router Cache (mod.rs:248)

Minor Issues & Suggestions

6. OpenAPI Spec Typo (Already noted by ellipsis-dev)

7. Missing Loading State in UI (routes/+page.svelte:362)

8. Good Practices Observed ✅

Security Assessment

Code Quality

Testing Recommendations

Reproduction Instructions for Testing

Recommendation

Uh oh!

claude bot Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

claude bot Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

claude bot Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

claude bot Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

claude bot Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

claude bot Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

claude bot Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

claude bot Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

hugocasa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hugocasa Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

cloudflare-workers-and-pages bot commented Oct 28, 2025 •

edited

Loading

claude bot commented Nov 6, 2025 •

edited

Loading

hugocasa Nov 7, 2025 •

edited

Loading

hugocasa Nov 7, 2025 •

edited

Loading