Skip to content

Conversation

@icecrasher321
Copy link
Collaborator

@icecrasher321 icecrasher321 commented Dec 18, 2025

Summary

  • Inactivity Polling have to check if the trigger block is in active deployment version
  • ALL_TRIGGER_TYPES to be reused everywhere instead of hardcoded list
  • Filter parsing was not consistent with db default for no trigger case
  • Notif modal needs to show slack account display name in the same way as cred selector

Type of Change

  • Bug fix

Testing

Tested manually with @aadamgough

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

@vercel
Copy link

vercel bot commented Dec 18, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Review Updated (UTC)
docs Skipped Skipped Dec 18, 2025 8:44pm

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Dec 18, 2025

Greptile Summary

Consolidated trigger type definitions by introducing ALL_TRIGGER_TYPES constant to replace hardcoded lists across the codebase. Fixed filter parsing consistency by removing optional chaining (?.includes()) in favor of direct array access, ensuring filters always use DB defaults. Enhanced inactivity polling to verify trigger blocks exist in active deployment versions before checking workflow activity. Added Slack account display names using user email for consistency with credential selector.

  • Centralized trigger type definitions with ALL_TRIGGER_TYPES constant
  • Fixed filter parsing to be consistent with database defaults (non-null arrays)
  • Inactivity polling now checks active deployment versions for matching trigger types
  • Alert rules now filter historical queries by trigger type
  • Slack notifications display user email instead of account ID

Confidence Score: 4/5

  • This PR is safe to merge with minor considerations around filter consistency
  • The changes consolidate trigger types and fix filter parsing logic. The removal of optional chaining relies on DB defaults always being present, which is correct per the schema. The inactivity polling logic adds important filtering by deployment state. All changes are well-structured and tested.
  • apps/sim/lib/logs/events.ts - verify filter arrays are always populated from DB defaults

Important Files Changed

Filename Overview
apps/sim/lib/logs/types.ts Added ALL_TRIGGER_TYPES constant and TriggerType to centralize trigger type definitions
apps/sim/lib/logs/events.ts Changed filter parsing from optional (?.includes()) to required (.includes()), added triggerFilter to alert context
apps/sim/lib/notifications/alert-rules.ts Added triggerFilter parameter to all alert check functions to filter database queries by trigger types
apps/sim/lib/notifications/inactivity-polling.ts Added filtering logic to only check workflows with matching trigger types in active deployment versions

Sequence Diagram

sequenceDiagram
    participant WF as Workflow Execution
    participant EE as Event Emitter
    participant DB as Database
    participant AR as Alert Rules
    participant IP as Inactivity Polling
    participant ND as Notification Delivery

    Note over WF,ND: Workflow Execution Flow
    WF->>EE: emitWorkflowExecutionCompleted(log)
    EE->>DB: Query subscriptions for workspace
    DB-->>EE: Return active subscriptions
    
    loop For each subscription
        EE->>EE: Check levelFilter.includes(log.level)
        EE->>EE: Check triggerFilter.includes(log.trigger)
        alt Filters match
            EE->>AR: shouldTriggerAlert(config, context, lastAlertAt)
            AR->>DB: Query logs with triggerFilter
            Note over AR,DB: Queries now filtered by trigger types:<br/>consecutive_failures, failure_rate,<br/>latency_spike, error_count
            DB-->>AR: Historical execution data
            AR-->>EE: Alert decision
            alt Should alert
                EE->>DB: Update lastAlertAt
                EE->>ND: Create notification delivery
            end
        end
    end

    Note over IP,ND: Inactivity Polling (Hourly)
    IP->>DB: Query no_activity subscriptions
    DB-->>IP: Return subscriptions
    
    loop For each subscription
        IP->>IP: Validate triggerFilter exists
        IP->>DB: Get workflows with active deployments
        DB-->>IP: Deployed workflows with state
        IP->>IP: deploymentHasTriggerType(state, triggerFilter)
        Note over IP: Filters workflows by checking if<br/>deployment has matching trigger blocks
        IP->>DB: Query recent logs with triggerFilter
        DB-->>IP: Recent execution logs
        alt No activity in window
            IP->>DB: Update lastAlertAt
            IP->>ND: Create notification delivery
        end
    end
Loading

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional Comments (1)

  1. apps/sim/lib/notifications/inactivity-polling.ts, line 112-113 (link)

    logic: triggerFilter and levelFilter could be null, causing inArray to fail

7 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@icecrasher321
Copy link
Collaborator Author

@greptile

@icecrasher321 icecrasher321 merged commit 04cd837 into staging Dec 18, 2025
11 checks passed
@icecrasher321 icecrasher321 deleted the notifs/fixes branch December 18, 2025 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants