Skip to content

Conversation

yileicn
Copy link
Collaborator

@yileicn yileicn commented Sep 10, 2025

PR Type

Enhancement


Description

  • Add debug configuration for crontab rule triggering

  • Implement conditional rule execution in debug mode

  • Introduce DebugSetting class with AllowRuleTrigger property


Diagram Walkthrough

flowchart LR
  A["CrontabSettings"] --> B["DebugSetting"]
  B --> C["AllowRuleTrigger property"]
  C --> D["CrontabWatcher debug logic"]
  D --> E["Conditional rule execution"]
Loading

File Walkthrough

Relevant files
Configuration changes
CrontabSettings.cs
Add debug settings configuration class                                     

src/Infrastructure/BotSharp.Abstraction/Crontab/Settings/CrontabSettings.cs

  • Add Debug property of type DebugSetting to CrontabSettings
  • Create new DebugSetting class with AllowRuleTrigger string property
+6/-0     
Enhancement
CrontabWatcher.cs
Implement conditional debug rule execution                             

src/Infrastructure/BotSharp.Core.Crontab/Services/CrontabWatcher.cs

  • Modify debug mode logic to check AllowRuleTrigger setting
  • Only execute crontab event if rule title matches configured trigger
+4/-1     

Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Null Reference Risk

Accessing settings.Debug.AllowRuleTrigger assumes settings and settings.Debug are non-null; consider null checks or safe navigation to avoid potential NRE when debug config is missing.

if (item.Title == settings.Debug.AllowRuleTrigger)
{
    await HandleCrontabEvent(item);
}                    
Debug Behavior Change

In DEBUG builds, rules only execute when the title exactly equals AllowRuleTrigger; previously all matching rules executed. Confirm this intentional change and consider logging when filtered out to aid debugging.

#if DEBUG
                    if (item.Title == settings.Debug.AllowRuleTrigger)
                    {
                        await HandleCrontabEvent(item);
                    }                    
#else
Config Validation

AllowRuleTrigger is a free-form string; consider trimming/normalizing and documenting expected rule title casing to avoid unexpected mismatches.

public class DebugSetting
{
    public string AllowRuleTrigger { get; set; } = "";
}

Copy link

qodo-merge-pro bot commented Sep 10, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Null-safe, case-insensitive comparison

Guard against a potential null Debug section to avoid a NullReferenceException
if configuration binds it to null. Also use a case-insensitive, trimmed
comparison to prevent accidental mismatches due to casing or whitespace.

src/Infrastructure/BotSharp.Core.Crontab/Services/CrontabWatcher.cs [91-96]

 #if DEBUG
-                    if (item.Title == settings.Debug.AllowRuleTrigger)
+                    var allowed = settings.Debug?.AllowRuleTrigger;
+                    if (!string.IsNullOrWhiteSpace(allowed) &&
+                        string.Equals(item.Title?.Trim(), allowed.Trim(), StringComparison.OrdinalIgnoreCase))
                     {
                         await HandleCrontabEvent(item);
-                    }                    
+                    }
 #else
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly points out that using == for string comparison is case-sensitive and can be brittle. Using a case-insensitive, null-safe comparison with trimming improves the robustness and reliability of this debug-only feature.

Medium
  • Update

@adenchen123
Copy link
Contributor

Reivewed

@yileicn yileicn merged commit 11d8580 into SciSharp:master Sep 10, 2025
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants